Python splines #1515
Python splines #1515
Conversation
|
ping @lcontento |
src/model.cpp
Outdated
| if (splines_[ispl].get_logarithmic_parametrization()) { | ||
| sspl_data[ispl + nspl * ip] = | ||
| state_.spl_[ispl] * splines_[ispl].getSensitivity(t, ip); | ||
| } else { | ||
| sspl_data[ispl + nspl * ip] = | ||
| splines_[ispl].getSensitivity(t, ip); | ||
| } |
There was a problem hiding this comment.
Can/should that if not go into getSensitivity?
There was a problem hiding this comment.
If we put the if inside then we would have to recompute the spline value for each parameter, while in this way we can recycle it.
There was a problem hiding this comment.
As mentioned in the other comment, I honestly think the if belongs logically inside the spline methods, but with the current method signature I would not move it. If we had a method that computes both spline value and spline sensitivities at the same time it would be better though in my opinion.
There was a problem hiding this comment.
see comment regarding same problem in sensitivity computation
There was a problem hiding this comment.
If I understand it correctly, this could be addressed by having get_{value,sensitivity} and get_unscaled_{value,sensitivity}? Here, we'd use the former, and where required inside splinesfunctions.cpp, we'd use the latter?
src/model.cpp
Outdated
| if (splines_[ispl].get_logarithmic_parametrization()) { | ||
| state_.spl_[ispl] = std::exp(splines_[ispl].getValue(t)); | ||
| } else { | ||
| state_.spl_[ispl] = splines_[ispl].getValue(t); | ||
| } |
There was a problem hiding this comment.
Can/should that if not go into getValue?
There was a problem hiding this comment.
I think it can and I think it should, not sure if there is any reason why Paul put it outside.
I can try putting inside getValue and see if anything breaks.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #1515 +/- ##
===========================================
+ Coverage 75.56% 76.06% +0.50%
===========================================
Files 76 80 +4
Lines 13126 14679 +1553
===========================================
+ Hits 9919 11166 +1247
- Misses 3207 3513 +306
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Make _print_with_exception, _get_sym_lines_array, and _get_sym_lines_symbols class methods of ODEExporter, as they may need to consider model-specific user_functions. Prerequisite for #1515.
* Use class instead of dict for function info, to simplify default argument handling and to find more easily where the respective values are used. * Allow specifying non-void return type. * Remove redundant virtual with override * Cleanup. Will simplify some changes in #1515
* Refactor ODE export * Use class instead of dict for function info, to simplify default argument handling and to find more easily where the respective values are used. * Allow specifying non-void return type. * Remove redundant virtual with override * Cleanup. Will simplify some changes in #1515 Co-authored-by: Fabian Fröhlich <[email protected]>
…I-dev/AMICI into python_splines_before_merge
This reverts commit ac596be.
This reverts commit 5007143.
…I-dev/AMICI into python_splines_before_merge
This reverts commit 4bef57a.
|
SonarCloud Quality Gate failed. |
This reverts commit 9eedc54.
This one is to replace #1245. However, it seems like in the long time of divergence, there were changes to functions from sbml_import, that may be not captured, as these functions have been moved out of sbml_import.py to sbml_utils.py on the python_splines branch... It looks like automatic merge of these files did not do completely the right thing... So, we have to be careful about functions that have been moved to newly created files, it seems...