Conversation
rozyczko
added
[scope] enhancement
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #172 +/- ##
===========================================
+ Coverage 81.15% 81.68% +0.52%
===========================================
Files 52 55 +3
Lines 4267 4516 +249
Branches 740 785 +45
===========================================
+ Hits 3463 3689 +226
- Misses 624 639 +15
- Partials 180 188 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
henrikjacobsenfys
left a comment
henrikjacobsenfys
left a comment
There was a problem hiding this comment.
Just a few questions, otherwise looks good to me
damskii9992
left a comment
damskii9992
left a comment
There was a problem hiding this comment.
Can you please split this PR into two? I believe the ModelCollection needs its own PR and ADR discussion.
|
|
||
| def __init__( | ||
| self, | ||
| model: NewBase, |
There was a problem hiding this comment.
The typehint should be ModelBase for now and should probably be SampleModelBase when that base class has been properly updated.
| **kwargs : Any | ||
| Additional calculator-specific options. | ||
| """ | ||
| if model is None: |
There was a problem hiding this comment.
why not if not isinstance(ModelBase)?
| self._additional_kwargs = kwargs | ||
| # Register this calculator and model in the global object map | ||
| if hasattr(model, 'unique_name'): | ||
| self._global_object.map.add_edge(self, model) |
There was a problem hiding this comment.
So, if we know that the model is a ModelBase (or even NewBase), then we know it has a unique_name attribute and this check is redundant. But also, why add the edge? As far as I know, we don't use these "edges" in the map anywhere, and I hope to remove it entirely, so unless you have a reason to add it, please don't.
| List[str] | ||
| Names of all calculators that can be created by this factory. | ||
| """ | ||
| ... |
There was a problem hiding this comment.
I think we can do better than this in the base class. How about if the factory has an internal attribute _available_calculators which is an empty list and a generic method which attempts to import a package based on input, and if successful adds it to this list. That way we can have automatic granularity of installed calculators and even if a calculator for some reason fails on the users end we can still support the other calculators.
| """Find a parameter by its serializer_id from all parameters in the global map.""" | ||
| for obj in self._global_object.map._store.values(): | ||
| for key in self._global_object.map.vertices(): | ||
| obj = self._global_object.map.get_item_by_key(key) |
There was a problem hiding this comment.
Haha, I knew I remembered seeing that you used the internal _store somewhere :D
There was a problem hiding this comment.
shhh... you didn't SEE ANYTHING
I can but the subsequent merge of these two would be ugly as the functionality is overlapping to some extent. |
3 participants
This PR implements the architecture decision from Discussion #160 to introduce a new stateless calculator factory pattern for physics calculators in EasyScience.
Key Components Added
Files Modified