Skip to content
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

Add support for observable-dependent sigmas #1692

Merged
merged 24 commits into from
Mar 4, 2022

Conversation

dweindl
Copy link
Member

@dweindl dweindl commented Mar 1, 2022

Standard deviations may now depend on observables.

Side-effects:

  • ssigmay will not be available anymore with adjoint sensitivity analysis.
  • Observable IDs shadowing other model entities are no longer allowed

Closes #609

Standard deviations may now depend on observables.

Closes #609
@dweindl dweindl force-pushed the feature_609_multiplicative_noise branch from 3b30e10 to 41ff9b9 Compare March 1, 2022 22:32
@codecov
Copy link

codecov bot commented Mar 1, 2022

Codecov Report

Merging #1692 (6be7332) into develop (3c6bd41) will increase coverage by 0.07%.
The diff coverage is 92.30%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1692      +/-   ##
===========================================
+ Coverage    78.38%   78.45%   +0.07%     
===========================================
  Files           70       70              
  Lines        11371    11414      +43     
===========================================
+ Hits          8913     8955      +42     
- Misses        2458     2459       +1     
Flag Coverage Δ
cpp 75.51% <91.66%> (+0.12%) ⬆️
petab 62.90% <100.00%> (+0.01%) ⬆️
python 68.49% <100.00%> (+0.01%) ⬆️
sbmlsuite 88.96% <0.00%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
include/amici/abstract_model.h 0.00% <ø> (ø)
include/amici/model.h 60.86% <ø> (ø)
python/amici/ode_export.py 93.86% <ø> (ø)
src/abstract_model.cpp 4.95% <0.00%> (-0.11%) ⬇️
src/model.cpp 84.99% <95.12%> (+0.32%) ⬆️
python/amici/sbml_import.py 95.97% <100.00%> (+0.01%) ⬆️
src/rdata.cpp 93.12% <100.00%> (ø)
src/sundials_matrix_wrapper.cpp 82.56% <0.00%> (+0.68%) ⬆️

@dweindl dweindl marked this pull request as draft March 1, 2022 23:29
@dweindl dweindl marked this pull request as ready for review March 3, 2022 21:27
@dweindl dweindl requested a review from FFroehlich March 3, 2022 21:27
Copy link
Member

@FFroehlich FFroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Should be possible to add Raia model (https://github.com/Benchmarking-Initiative/Benchmark-Models-PEtab/tree/model_Raia_CancerResearch2011) to bmc tests now, right?

src/model.cpp Outdated
auto ret = SUNMatScaleAdd(1.0, derived_state_.dJydy_.at(iyt).get(),
tmp_sparse);
if(ret != SUNMAT_SUCCESS) {
SUNMatDestroy(tmp_sparse);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move SUNMatDestroy(tmp_sparse); before if clause?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely.

@@ -142,7 +142,6 @@ void ReturnData::initializeFullReporting(bool quadratic_llh) {
srz.resize(nmaxevent * nz * nplist, 0.0);
}

ssigmay.resize(nt * ny * nplist, 0.0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't delete but only initialize if sensi_meth == SensitivityMethod::forward?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's already taken care of by ReturnData::initializeResidualReporting which is called a few lines above.

AMICI/src/rdata.cpp

Lines 75 to 86 in 353e7f9

void ReturnData::initializeResidualReporting(bool enable_res) {
y.resize(nt * ny, 0.0);
sigmay.resize(nt * ny, 0.0);
if (enable_res)
res.resize((sigma_res ? 2 : 1) * nt * nytrue, 0.0);
if ((sensi_meth == SensitivityMethod::forward &&
sensi >= SensitivityOrder::first)
|| sensi >= SensitivityOrder::second) {
sy.resize(nt * ny * nplist, 0.0);
ssigmay.resize(nt * ny * nplist, 0.0);

@dweindl
Copy link
Member Author

dweindl commented Mar 4, 2022

Nice! Should be possible to add Raia model (https://github.com/Benchmarking-Initiative/Benchmark-Models-PEtab/tree/model_Raia_CancerResearch2011) to bmc tests now, right?

Thanks for your help.

Raia_CancerResearch2011 can now be imported. However, the computed likelihood is very different from

Raia_CancerResearch2011:
llh: 690.619495552297
.

dweindl added 2 commits March 4, 2022 12:02
…led.xml history in ./python/examples/example_steadystate/model_steadystate_scaled_without_observables.xml history.
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 4, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

5.9% 5.9% Coverage
0.5% 0.5% Duplication

@dweindl dweindl merged commit 7168f56 into develop Mar 4, 2022
@dweindl dweindl deleted the feature_609_multiplicative_noise branch March 4, 2022 14:22
@dweindl dweindl mentioned this pull request Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants