Add parameters class #322
Add parameters class #322
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #322 +/- ##
===========================================
+ Coverage 95.55% 97.13% +1.58%
===========================================
Files 40 41 +1
Lines 2203 2305 +102
===========================================
+ Hits 2105 2239 +134
+ Misses 98 66 -32 ☔ View full report in Codecov by Sentry. |
|
In this implementation, I have kept the I think it would be good to move towards a solution where the user only needs to interact with |
|
All tests should pass after #321 is merged. |
There was a problem hiding this comment.
This is looking very good @NicolaCourtier. I've left some suggestions and comments for you to consider.
| else: | ||
| self.sigma0 = sigma | ||
| self.sigma = sigma |
There was a problem hiding this comment.
For consistency, we should move these to all be self.sigma0 instead of self.sigma. I was loose with this initial creation, but they should be consistent with the rest of the codebase. The same goes for the sigma variables.
pybop/parameters/parameter.py
Outdated
|
|
||
| self.update_bounds() | ||
|
|
||
| def update_bounds(self): |
There was a problem hiding this comment.
If we add an optional arg here and inspect it on entry, we can add functionality to either iterate over the entire dict and update bounds, or just for the arg (parameter) passed. This would be helpful for the above update_bounds and remove_bounds, as they currently iterate over the parameters on order O(N!) O(n(n+1)/2).
There was a problem hiding this comment.
I've switched update_bounds to the more accurate get_bounds and removed the property bounds on the Parameters class. We could implement an iterative set_bounds in future but for now the functionality of set_bounds relates to an individual Parameter.
|
|
||
| @property | ||
| def n_parameters(self): | ||
| return self._n_parameters | ||
| return len(self.parameters) |
There was a problem hiding this comment.
The update to n_parameters across the codebase has us recalculating len(self.parameters) quite a bit. I think this is fine since len(self.parameters) is inheritantly very small, but it's worth noting for the future that this could be an issue with larger objects (or if self.parameters) becomes very large.
There was a problem hiding this comment.
We should consider moving the property onto the Parameters class
| sigma0.append(param.prior.sigma) | ||
| else: | ||
| all_have_sigma = False | ||
| if not all_have_sigma: |
There was a problem hiding this comment.
I wonder if a better implementation would be to insert either Nan or np.inf and return the list. This would ensure that we get a value ofsigma0 for all defined parameters.
There was a problem hiding this comment.
Good idea, let's implement and test this in another PR
Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>
…PyBOP into 258-parameters-class
Description
Add the Parameters class to store and access multiple parameters in one object.
Issue reference
Fixes #258
Review
Before you mark your PR as ready for review, please ensure that you've considered the following:
Type of change
Key checklist:
$ pre-commit run(or$ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ nox -s tests$ nox -s doctestYou can run integration tests, unit tests, and doctests together at once, using
$ nox -s quick.Further checks:
Thank you for contributing to our project! Your efforts help us to deliver great software.