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

dilpath · 2024-07-02T22:10:41Z

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.

…dict` with amici posterior

codecov-commenter · 2024-07-02T22:13:17Z

⚠️ Please install the 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 ⚠️

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

Additional details and impacted files

@@             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

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

PaulJonasJost

A test for this would be good as well ensuring it works as intended?

PaulJonasJost · 2024-07-03T07:40:16Z

pypesto/objective/amici/amici.py

@@ -458,18 +439,23 @@ def call_unprocessed(

        x_dct = self.par_arr_to_dct(x)

-        # only ask amici to compute required quantities
        amici_reporting = (
            self.amici_reporting
            if amici_reporting is None
            else amici_reporting
        )
        if amici_reporting is None:


It should be made clear in the documentation that amici_reporting takes precedence over return_dict imo.

PaulJonasJost · 2024-07-03T07:42:41Z

pypesto/objective/base.py

+        if self.share_return_dict:
+            kwargs["return_dict"] = return_dict


What if return_dict is in kwargs already?

It cannot be, since return_dict is it's own kwarg in this method's argument list.

PaulJonasJost · 2024-07-03T07:43:27Z

pypesto/objective/aggregated.py

+            if objective_.share_return_dict:
+                objective_kwargs["return_dict"] = return_dict


Same as in the base objective (see above or below comment)

I would actually argue that the outer return_dict should take precedence over inner return_dict and that we don't need the if objective_.share_return_dict: check at all.

FFroehlich · 2024-07-03T07:53:13Z

pypesto/objective/base.py

@@ -178,6 +183,8 @@ def __call__(
        x_full = self.pre_post_processor.preprocess(x=x)

        # compute result
+        if self.share_return_dict:


why is this necessary?

Otherwise there might be some argument error, because return_dict is not in every custom user objective.call_unprocessed signature. But I'll switch to Daniel's suggestion now

FFroehlich · 2024-07-03T07:55:10Z

pypesto/objective/aggregated.py

+            if objective_.share_return_dict:
+                objective_kwargs["return_dict"] = return_dict


I would actually argue that the outer return_dict should take precedence over inner return_dict and that we don't need the if objective_.share_return_dict: check at all.

dweindl · 2024-07-03T08:03:25Z

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.

dweindl

Thx

stephanmg

Looks fine, many thanks for providing this fix.

dilpath · 2024-07-03T09:02:55Z

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.

add ObjectiveBase.share_return_dict; small refactor to fix `return_…

5860c0f

…dict` with amici posterior

dilpath requested a review from stephanmg July 2, 2024 22:10

dilpath requested review from dweindl, FFroehlich and PaulJonasJost as code owners July 2, 2024 22:10

PaulJonasJost approved these changes Jul 3, 2024

View reviewed changes

FFroehlich approved these changes Jul 3, 2024

View reviewed changes

use user-supplied return_dict

27779ab

dweindl approved these changes Jul 3, 2024

View reviewed changes

test share_return_dict

1ff18a9

dilpath requested a review from m-philipps as a code owner July 3, 2024 08:14

stephanmg approved these changes Jul 3, 2024

View reviewed changes

dilpath and others added 3 commits July 3, 2024 10:47

make default with DeprecationWarning

5a592e1

add return_dict to all call_unprocessed signatures

0a4cc11

Merge branch 'develop' into fix_amici_posterior_return_dict

8b21c42

fix FDobjective

b9ec235

dilpath merged commit 10d6092 into develop Jul 3, 2024
18 checks passed

dilpath deleted the fix_amici_posterior_return_dict branch July 3, 2024 10:43

This was referenced Jul 25, 2024

Release 0.5.3 #1432

Merged

Prepare release v0.5.3 #1434

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dilpath commented Jul 2, 2024

codecov-commenter commented Jul 2, 2024 •

edited

Loading

PaulJonasJost left a comment

PaulJonasJost Jul 3, 2024

PaulJonasJost Jul 3, 2024

dilpath Jul 3, 2024

PaulJonasJost Jul 3, 2024

FFroehlich Jul 3, 2024

FFroehlich Jul 3, 2024

dilpath Jul 3, 2024

FFroehlich Jul 3, 2024

dweindl commented Jul 3, 2024

dweindl left a comment

stephanmg left a comment

dilpath commented Jul 3, 2024

		if self.share_return_dict:
		kwargs["return_dict"] = return_dict

		if objective_.share_return_dict:
		objective_kwargs["return_dict"] = return_dict

Fix return_dict when an AmiciObjective is inside AggregatedObjective #1424

Fix return_dict when an AmiciObjective is inside AggregatedObjective #1424

Conversation

dilpath commented Jul 2, 2024

codecov-commenter commented Jul 2, 2024 • edited Loading

Codecov Report

PaulJonasJost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dweindl commented Jul 3, 2024

dweindl left a comment

Choose a reason for hiding this comment

stephanmg left a comment

Choose a reason for hiding this comment

dilpath commented Jul 3, 2024

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

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

codecov-commenter commented Jul 2, 2024 •

edited

Loading