-
Notifications
You must be signed in to change notification settings - Fork 7
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
Refactor priors #329
base: develop
Are you sure you want to change the base?
Refactor priors #329
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #329 +/- ##
===========================================
+ Coverage 74.51% 74.56% +0.05%
===========================================
Files 53 54 +1
Lines 5222 5378 +156
Branches 910 923 +13
===========================================
+ Hits 3891 4010 +119
- Misses 984 1016 +32
- Partials 347 352 +5 ☔ View full report in Codecov by Sentry. |
5313345
to
b28e5c1
Compare
721ba31
to
d511639
Compare
d511639
to
9381d7c
Compare
Requires some clarification related to normalization of the prior density. Related to PEtab-dev/PEtab#402. EDIT: Needs clarification, but the current implementation matches what was done before. |
a272255
to
86643ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks 👍
petab/v1/distributions.py
Outdated
|
||
""" | ||
|
||
_type_to_cls: dict[str, type[Distribution]] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc/comment
petab/v1/distributions.py
Outdated
def __init__( | ||
self, | ||
type_: str, | ||
transformation: str, | ||
parameters: tuple, | ||
bounds: tuple = None, | ||
parameter_scale: bool = False, | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Support supplying some rng: np.Generator = None
and otherwise set self.rng = np.random.default_rng()
in __init__
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I'll add that separately. For now, it's handled the same way as before, using the default numpy random state.
petab/v1/distributions.py
Outdated
parameter_scale=_parameter_scale, | ||
type_=_type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why _
both before and after?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type_
to not shadow builtin type()
; _type
, private, it should not be used explicitly be the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then remove from __init__
signature? Or change to be private class attributes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It required for initialization. Moving it to an attribute is not really helpful. After further refactoring it's gone completely.
petab/v1/sampling.py
Outdated
@@ -26,84 +26,10 @@ def sample_from_prior( | |||
""" | |||
# unpack info | |||
p_type, p_params, scaling, bounds = prior | |||
prior_cls = Distribution._type_to_cls[p_type] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is _type_to_cls
private? Could remove the _
prefix otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'd like to decrease the API surface. Unless there is a clear use case, I'd consider things private to simplify refactoring. Here, I don't see a clear use case yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, my question was rather because _type_to_cls
is used outside of Distribution
here, but fine to use _*
for things that can be used anywhere in the library but not "outside" the library, e.g. in user code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's completely gone by now anyways, will update in a sec.
|
||
# Kolmogorov-Smirnov test to check if the sample is drawn from the CDF | ||
_, p = kstest(sample, cdf) | ||
assert p > 0.05, (p, distribution) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be higher than 0.05
? e.g. p > 0.5
should also always work for 10000 samples, just guessing. Anyway, fine as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With p > 0.5 this will fail frequently. Even with p > 0.05 it fails often enough so that I set a seed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's surprising! I would have guessed that 10,000 samples from a "simple" distribution should be enough to say "these distributions look similar"... anyway fine 👍
a6429c8
to
9013648
Compare
Introduce `Distribution` classes for handling prior distributions and sampling from them. Later on this can be extended to noise models for measurements.
9013648
to
e25785f
Compare
0b704ba
to
cc6a7e7
Compare
cc6a7e7
to
047dc46
Compare
Introduces
Distribution
classes for handling prior distributions, sampling from them, and evaluating negative log-priors (#312). Later on, this can be extended to noise models for measurements and computing loglikelihoods.This also adds a notebook demonstrating the various prior options which are a common source confusion, and fixes a faulty test.
Closes #311.
👀 notebook: https://petab--329.org.readthedocs.build/projects/libpetab-python/en/329/example/distributions.html