Pass Inputs dictionary #359
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #359 +/- ##
===========================================
+ Coverage 97.49% 97.60% +0.10%
===========================================
Files 42 42
Lines 2471 2459 -12
===========================================
- Hits 2409 2400 -9
+ Misses 62 59 -3 ☔ View full report in Codecov by Sentry. |
|
The diff of this PR is 100% covered and I've added a number of tests but the project coverage is still down by 0.11%. I think it might worth continuing with the review despite this. |
|
Note that there will be conflicts with #352, as mentioned here: #352 (comment) These conflicts have now been resolved, update here: #352 (comment) |
|
Thanks for the helpful chat @BradyPlanden! I've added a |
| for param in parameters: | ||
| if param not in self.param.values(): | ||
| self.add(param) | ||
| else: | ||
| print(f"Discarding duplicate {param.name}.") |
There was a problem hiding this comment.
Context: I am coming from the MultiFitting PR. If I understand this correctly, if a user provides the same parameter for 2 problems in the MultiFitting, only the first instance will make it through while the others will be discarded. This is fine if the users gives us the exact same 2 parameters (with bounds and initial values) but otherwise I think it would cause issues. I can think of 2 ways around this:
- Throw an error if a parameter appears twice with different bounds/initial values.
- Combine the two parameters somehow (take intersection between domains of validity?) and throw a warning to let the user know.
There was a problem hiding this comment.
Thanks for the feedback @brosaplanella! At the moment, it is option 1 because adding a parameter with the same name will return an error (see parameter.py line 223) - even if the initial values and bounds are identical. In future, we could upgrade to option 2, but 1 seems the more controlled approach for now.
There was a problem hiding this comment.
Thanks for the additions @NicolaCourtier. A few comments to look at, but I think it's heading in a good direction.
| @@ -322,13 +332,6 @@ class RandomClass: | |||
| with pytest.raises(ValueError): | |||
| pybop.Optimisation(cost=cost, optimiser=RandomClass) | |||
|
|
|||
| @pytest.mark.unit | |||
| def test_prior_sampling(self, cost): | |||
There was a problem hiding this comment.
This is a pretty pedantic test, but do we replicate this elsewhere?
There was a problem hiding this comment.
The prior sampling is carried out by the rvs function of a Parameter, so I believe this is tested in test_priors.py.
Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>
Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>
|
Hi @BradyPlanden, thanks for the review. The two failing integration tests also fail when run with matching parameter values on |
|
Noting that I applied the suggestions by @BradyPlanden to revert the passing of inputs as dictionaries, back to lists, in the tests. We will continue to support lists of parameter values, although it is safer to use dictionaries. The allowable Inputs are defined in Parameters |
Description
Aligning Inputs between problem, observer and model.
Issue reference
Fixes #358.
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.