ICB-DCM / pyPESTO

python Parameter EStimation TOolbox
https://pypesto.readthedocs.io
BSD 3-Clause "New" or "Revised" License
206 stars 44 forks source link

Fix `return_dict` when an `AmiciObjective` is inside `AggregatedObjective` #1424

Closed dilpath closed 5 days ago

dilpath commented 6 days ago

AmiciObjective previously returned all AMICI RData information when return_dict=True. However, this was handled via __call__. If the PEtab problem has a prior, then an AggregatedObjective is created, with an AmiciObjective and the prior inside. The AggregatedObjective calls call_unprocessed of each internal objective, skipping AmiciObjective.__call__ and hence one would see something like:

# call AggregatedObjective
>>> result = objective(x, return_dict=True)
>>> result['rdatas'][0].y is None
True
>>> result = objective(x, return_dict=True, amici_reporting=amici.RDataReporting.full)
>>> result['rdatas'][0].y is None
False

This PR changes the above to

>>> result = objective(x, return_dict=True)
>>> result['rdatas'][0].y is None
False

i.e., rdatas are now "full" and e.g. plotting is now possible.

An alternative fix would be to always pass return_dict to call_unprocessed. This would be fine for me, but might break someone's custom objective.

codecov-commenter commented 6 days ago

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

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.35%. Comparing base (5a8e014) to head (b9ec235).

Files Patch % Lines
pypesto/objective/aggregated.py 83.33% 1 Missing :warning:

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #1424 +/- ## =========================================== + Coverage 83.33% 83.35% +0.01% =========================================== Files 161 161 Lines 13226 13235 +9 =========================================== + Hits 11022 11032 +10 + Misses 2204 2203 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dweindl commented 5 days ago

An alternative fix would be to always pass return_dict to call_unprocessed. This would be fine for me, but might break someone's custom objective.

I think this would make sense. Your current implementation would already enable issuing a DeprecationWarning and updating things accordingly.

dilpath commented 5 days ago

An alternative fix would be to always pass return_dict to call_unprocessed. This would be fine for me, but might break someone's custom objective.

I think this would make sense. Your current implementation would already enable issuing a DeprecationWarning and updating things accordingly.

Thanks, I decided to simply switch to it by default now, with a DeprecationWarning.