add bounds argument and split log(a*exp(b)) #149
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #149 +/- ##
==========================================
+ Coverage 89.79% 89.82% +0.02%
==========================================
Files 9 9
Lines 1235 1238 +3
==========================================
+ Hits 1109 1112 +3
Misses 126 126 ☔ View full report in Codecov by Sentry. |
|
There was a problem hiding this comment.
Thanks for this contribution. I have visually checked the behavior of calc_overschrijding() and this still works as expected, with the improvement that the warning is not given. So your fix works beautifully. I have therefore updated some expected values in the tests. I have also accidentally reformatted the code with black which messes up the PR a bit, sorry about that. There is one suggestion about None instead of math.inf included below, if you agree with this please commit it as a change and after that (or otherwise) please go ahead and merge the PR.
Normally I would update the changelog (docs/whats-new.md), but since this fix does not impact "the user" I think that can be skipped.
| @@ -339,6 +341,7 @@ def cost_func(params, *args): | |||
| x0=initial_guess, | |||
| args=(p_val_gt_threshold, threshold, values[values > threshold]), | |||
| method="Nelder-Mead", | |||
| bounds = [[-math.inf, math.inf], [1e-10, math.inf]], | |||
There was a problem hiding this comment.
I think math.inf can be replaced with None which might be slightly neater since it reverts to the default bounds in that case (which are most probably +/- math.inf)
There was a problem hiding this comment.
I tried a few thinks like you suggested, but ended up with errors as: 'NoneType' object is not iterable, so I keep it as it is.
There was a problem hiding this comment.
Maybe I should have been more clear, my suggestion was to use bounds = [[None, None], [1e-10, None]],. I think I successfully ran the testbank with this, but you might have done another test? Either way, since the PR is already merged I thing it is fine to leave it as is for now. Thanks for fixing this issue!
No description provided.