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

AmiciObjective/PEtab import: Fix plist #1493

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

dweindl
Copy link
Member

@dweindl dweindl commented Oct 18, 2024

For further background, see #1448.

We don't have to compute the full gradient in every model simulation. Some entries might not be required because of fixed parameter or some condition-specific objective parameter mapping. The former was supported (however, not tested), the latter was not supported. Now both are tested an supported.

There was no good way to communicate the fixed parameters to AmiciCalculator where ExpData.plist gets set. Passing this information is currently only possible through the parameter mapping, based on which the required sensitivities are determined. Therefore, in addition to the general parameter mapping, there is now a parameter mapping that accounts for the fixed parameters. Not sure what could be a more elegant way to handle that.

@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.75%. Comparing base (47ae5d5) to head (ee11bd6).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1493      +/-   ##
===========================================
+ Coverage    82.74%   82.75%   +0.01%     
===========================================
  Files          163      163              
  Lines        13892    13903      +11     
===========================================
+ Hits         11495    11506      +11     
  Misses        2397     2397              

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

@dweindl dweindl marked this pull request as ready for review October 19, 2024 08:26
@dweindl dweindl self-assigned this Oct 19, 2024
For further background, see ICB-DCM#1448.

We don't have to compute the full gradient in every model simulation. Some entries might not be required because of fixed parameter or some condition-specific objective parameter mapping. The former was supported (however, not tested), the latter was not supported. Now both are tested an supported.

There was no good way to communicate the fixed parameters to AmiciCalculator where ExpData.plist gets set. Passing this information is currently only possible through the parameter mapping, based on which the required sensitivities are determined. Therefore, in addition to the general parameter mapping, there is now a parameter mapping that accounts for the fixed parameters. Not sure what could be a more elegant way to handle that.
Copy link
Collaborator

@PaulJonasJost PaulJonasJost left a comment

Choose a reason for hiding this comment

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

Looks good to me. Also would not be aware of a better suitable/more elegant approach

test/petab/test_petab_import.py Show resolved Hide resolved
pypesto/objective/amici/amici.py Show resolved Hide resolved
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.

3 participants