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

Treat observableParameter overrides as placeholders in noise formulae #231

Merged
merged 4 commits into from
Dec 12, 2023

Conversation

dilpath
Copy link
Member

@dilpath dilpath commented Dec 11, 2023

Currently, linting will fail if an observableParameter (e.g. a scaling or offset) appears in the noise formulae. Having the full observable formulae in the noise formulae is currently the only way to get observable-dependent noise -- this means noise formulae should support observableParameter placeholders.

This fixes the issue for this case: Benchmarking-Initiative/Benchmark-Models-PEtab#197

If I compare the AMICI model from these cases:

  • observable-dependent noise via observable ID in noise formulae (invalid PEtab, but compiles fine with AMICI)
  • this fix + observable-dependent noise via observable formulae in the noise formulae
    I get the same C++ files.

@dilpath dilpath requested a review from a team as a code owner December 11, 2023 18:20
dilpath added a commit to m-philipps/Benchmark-Models-PEtab that referenced this pull request Dec 11, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c0158ba) 76.28% compared to head (f907b2e) 76.28%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #231   +/-   ##
========================================
  Coverage    76.28%   76.28%           
========================================
  Files           34       34           
  Lines         3188     3188           
  Branches       774      774           
========================================
  Hits          2432     2432           
+ Misses         555      554    -1     
- Partials       201      202    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@dweindl dweindl left a comment

Choose a reason for hiding this comment

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

Thanks. Makes sense. Could you please add/extend a test?

We should probably also clarify that in https://github.com/PEtab-dev/PEtab/blob/main/doc/documentation_data_format.rst

@dilpath
Copy link
Member Author

dilpath commented Dec 12, 2023

Thanks. Makes sense. Could you please add/extend a test?

Done! I did not see a pre-existing test, so I created a new one in a test script that already provided a PEtab problem to work with. I changed this PEtab problem, because observable_1 is a parameter in the test problem model, and obs1 is the observable ID used in the measurements table.

We should probably also clarify that in https://github.com/PEtab-dev/PEtab/blob/main/doc/documentation_data_format.rst

Sounds good, done here: PEtab-dev/PEtab#574

@dilpath dilpath merged commit 9c23a21 into develop Dec 12, 2023
7 checks passed
@dilpath dilpath deleted the noise_formulae_observable_overrides branch December 12, 2023 22:16
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.

None yet

3 participants