-
Notifications
You must be signed in to change notification settings - Fork 45
Restructure priors #839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Restructure priors #839
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #839 +/- ##
===========================================
+ Coverage 89.41% 89.46% +0.04%
===========================================
Files 63 63
Lines 4857 4880 +23
===========================================
+ Hits 4343 4366 +23
Misses 514 514 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thank you @SarahRo, this is definitely going in the right direction. Thanks for highlighting two potential issues, I would say:
Also, please remove the |
Description
This PR introduces a clearer distinction between priors and distributions used for sampling initial conditions.
Moreover, the Parameter object is replaced with a ParameterInfo object and two subclasses (ParameterBounds and ParameterDistribution). This ensures that distributions cannot be set with conflicting bounds. If bounds are needed the distribution itself has to be bounded (for example, by using scipy.stats.truncate).
Renaming of classes and their methods and properties:
BasePrior->DistributionJointPrior->JointDistributionParameter->ParameterInfoParameter.sample_from_prior->ParameterInfo.sample_from_distributionParameter._prior->ParameterInfo._distributionParameter.prior->ParameterInfo.distributionParameters.sample_from_priors->Parameters.sample_from_distributionsParameters.priors ->Parameters.distributionsChanges to structure:
Parameteris replaced with a base class (ParameterInfo) and two subclasses (ParameterDistributionandParameterBounds).The constructor for
ParameterInfotakes neither a distribution (formerly prior) nor bounds as input. Bounds can be forced by using theset_boundsmethod.A
ParameterDistributionobject requires a distribution as input (either of typescipy.stats.distributions.rv_frozenorpybop.Distribution), but does not take bounds as input. Bounds are extracted from the distribution using thesupportmethod fromscipy.stats. Conflicting bounds can still be forced using theset_boundsmethod.A ParameterBounds object requires bounds as input. A distribution cannot be set by the user. For finite bounds, a uniform distribution is set automatically.
Changes to functionality:
pybop.Distributionobject (formerlypybop.BasePrior) is now constructed using a distribution of typescipy.stats.distributions.rv_frozenrather thanscipy.stats.rv_continuous. This means all parameters of the distribution are fixed by the user before passing it to the constructor of the class. The class no longer holds the values for thelocandscaleparemeters. This was necessary to allow for distributions that have additional input parameters (e.g.scipy.stats.truncnorm).pybop.Distributionis now implemented. This allows a user to initialise apybop.Distributionwith any distribution of typescipy.stats.distributions.rv_frozenand use a finite difference approximation of the derivative of the log distribution.Distribution.meanis now a method rather than a property so that it matches the same method forscipy.stats.distributions.rv_frozen.Distribution.sigmais replaced with the methodDistribution.std, to matchscipy.stats.distribtuions.rv_frozenDistribution.supportreplaceDistribution.bounds, again to matchscipy.stats.distributions.rv_frozen.__repr__has been modified forpybop.Distributionand its subclasses to match information held by the class.ParameterDistributionobject can be initialised with either apybop.Distributionor ascipy.stats.distribution.rv_frozen. Previously, aParameterrequired aBasePrior(nowDistribution).pybop.Gaussiancan now also be used for a truncated version of the normal distribution by providing the inputtruncated_at. The underlying distribution in this case isscipy.stats.truncnorm.Potential issues with these changes:
Distributionand aParameterDistributionobject might cause confusion.ParameterDistributionandParameterBoundsas subclasses, it would also be possible to simply modify the constructor ofParameterInfoto only allow either bounds or a distribution input. This could reduce complexity. The question here is, does the distinction betweenParameterDistributionandParameterBoundsimprove clarity or does it just introduce unnecessary complexity?Fixes # (issue)
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #).
Important checks:
Please confirm the following before marking the PR as ready for review:
$ pre-commit runor$ 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 testsnox -s doctest