implement support for parameterScale{Normal,Laplace} #520
Conversation
|
I added some documentation, support of |
|
Hence I will not review, since it now is also code of mine, but this looks fine to me :) |
|
remaining flake issue is already solved in #519 |
test/test_prior.py
Outdated
| @@ -13,20 +13,21 @@ | |||
| from pypesto.objective import NegLogParameterPriors | |||
| from pypesto.objective.priors import get_parameter_prior_dict | |||
|
|
|||
| scales = ['lin', 'log', 'log10'] | |||
There was a problem hiding this comment.
Similarly, these and other string constants in this file could be replaced with petab.C constants
There was a problem hiding this comment.
This would make the pypesto API dependent on petab, which I would rather avoid. It should be possible to use these functions without petab and we don't want that functionality to break just because petab changed some of its literals.
There was a problem hiding this comment.
Then it might be worthwhile for pyPESTO to maintain its own set of constants (no need now).
test/test_prior.py
Outdated
| @@ -68,8 +72,8 @@ def test_derivatives(): | |||
| Tests the finite gradients and second order derivatives. | |||
| """ | |||
|
|
|||
| scales = ['lin', 'log', 'log10'] | |||
| prior_types = ['uniform', 'normal', 'laplace', 'logNormal'] | |||
| prior_types = ['normal', 'laplace', 'logNormal', 'parameterScaleUniform', | |||
There was a problem hiding this comment.
Should 'uniform' be added here? If not, then if prior_type == 'uniform' could be removed (a few lines below this).
There was a problem hiding this comment.
refactored all of the test, uniform was added to that list, but the optimization test doesn't make too much sense since a flat function doesn't have local minima :D
There was a problem hiding this comment.
okay, well, it doesn't have a strict local minimum.
test/test_prior.py
Outdated
| assert abs(prior_dict['density_fun'](lin_to_scaled(1.5, scale)) | ||
| - math.log(1)) < 1e-8 | ||
| assert abs(prior_list[0]['density_fun']( | ||
| lin_to_scaled(1.5, scale)) - math.log(1) |
There was a problem hiding this comment.
| lin_to_scaled(1.5, scale)) - math.log(1) | |
| lin_to_scaled(1.5, scale)) |
test/test_prior.py
Outdated
| assert abs(prior_dict['density_fun'](lin_to_scaled(.5, scale)) | ||
| - 0) < 1e-8 | ||
| assert abs(prior_list[0]['density_fun']( | ||
| lin_to_scaled(.5, scale)) - 0 |
There was a problem hiding this comment.
| lin_to_scaled(.5, scale)) - 0 | |
| lin_to_scaled(.5, scale)) |
Are these - 0 or - math.log(1) from the old code intended?
There was a problem hiding this comment.
best to check via "diff < ...".
There was a problem hiding this comment.
Not sure what you are suggesting here.
test/test_prior.py
Outdated
| assert abs(prior_dict['density_fun'](lin_to_scaled(2.5, scale)) | ||
| - 0) < 1e-8 | ||
| assert abs(prior_list[0]['density_fun']( | ||
| lin_to_scaled(2.5, scale)) - 0 |
There was a problem hiding this comment.
| lin_to_scaled(2.5, scale)) - 0 | |
| lin_to_scaled(2.5, scale)) |
test/test_prior.py
Outdated
| assert abs(prior_dict['density_fun'](lin_to_scaled(.5, scale)) | ||
| - 0) < 1e-8 | ||
| assert abs(prior_list[0]['density_fun']( | ||
| lin_to_scaled(.5, scale)) - 0 |
There was a problem hiding this comment.
best to check via "diff < ...".
Co-authored-by: Yannik Schälte <31767307+yannikschaelte@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## develop #520 +/- ##
===========================================
+ Coverage 87.74% 90.71% +2.96%
===========================================
Files 65 65
Lines 4212 4212
===========================================
+ Hits 3696 3821 +125
+ Misses 516 391 -125
Continue to review full report at Codecov.
|
No description provided.