Feature spline regularization #1222
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Initial commit with spline regularization. The implementation of the gradient wrt theta is not completed (swapping to develop for some debugging of censored). Additionally, small fixes of censored gradient (sigma related, not important usually).
Implemented the complete gradient, including the regularization term for the ds_dtheta
FIxed the parameter plot issue (will be in a separate PR). Added the linear regularization to the spline visualization.
Changes that I might have to revert for the Pull request. The ensemble one might've already been pushed by Polina. The solver one completely circumvents dsdtheta gradient calculation due to some "array cannot be inf or NaN" error which pops sometimes, but it's mostly always 0 so removing it makes it easier to deal with for now. This is a big TODO, do not forget.
Fixed the par_sim_idx in the spline solver calculate gradient function. For all conditions, the rdata form is not managed by par_sim_idx, but with par_edata_indices. This is done such that if some parameters are not used in the condition (observable or noise parameters for instance) then the sensitivities wrt. them don't have to be calculated
Added the passing of the edata indices from the calculator to the solver gradient calculation. Additionally, fixed the inner calculator collector quantitative calculation with nansum instead of a regular one (sometimes it gave only nan values because of this...)
Since I've proven that the ds_dtheta gradient contribution is always 0, we can remove all of the code that was calculating it
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #1222 +/- ##
===========================================
- Coverage 88.16% 83.81% -4.36%
===========================================
Files 79 148 +69
Lines 5257 11948 +6691
===========================================
+ Hits 4635 10014 +5379
- Misses 622 1934 +1312 ☔ View full report in Codecov by Sentry. |
- Updated the notebook for non-linear semi-quantitative data - fixed the max_sensi_order problem - changed default min_diff_factor - fix quality
|
Tests done 👍 |
Doresic
requested review from
dweindl,
FFroehlich,
dilpath,
PaulJonasJost and
a team
as code owners
dweindl
approved these changes
Nov 30, 2023
There was a problem hiding this comment.
I'm not sufficiently familiar with the method, therefore just some general comments.
- Please add return type annotations where missing.
- https://pypesto--1222.org.readthedocs.build/en/1222/example/example_nonlinear_monotone.html
- The
visualize_optimized_model_fitfigure is hardly readable - Not sure whether there is a pypesto policy no notebooks, but I'd prefer clearing outputs before merging.
- The
dilpath
approved these changes
Nov 30, 2023
stephanmg
approved these changes
Dec 1, 2023
|
General suggestion: There seems to be some files (in the hierarchical folder) which have below 80% test coverage, perhaps you can improve this with meaningful tests. If not also fine. |
FFroehlich
approved these changes
Dec 4, 2023
Added tests for untested lines in calculators
PaulJonasJost
approved these changes
Dec 6, 2023
m-philipps
approved these changes
Dec 6, 2023
Yes, that would be good to have. I'll add tests in another PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The main focus of the PR: add spline regularization into the spline approximation method.
Other changed stuff:
parameter_ids_ensemblelist was not a list, so was failing at the.indexcall.calculate_quantitative_resultfunction of theinner_calculator_collector. It was giving allnanvalues, because the summation in the gradient calculation was not anp.nansum. That's fixed now.inner_calculator_collectorandspline_approximation.solvergradient calculation. Previously, it was usingpar_sim_idxas the index forrdata. However, it can happen that for some conditions not all parameters are used (i.e. observable parameters). In that case,edatashas aplistof indices of parameters used for that condition, which has to be used as indices for therdataobject.importer'screate_objectivefunction, if there are non-quantitative data types, themax_sensi_ordershould be set to 1, as higher-order derivatives are not implemented.parameters.pyrelated to hierarchical parameter plotting.Note: Still have TODO tests of the regularization.