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

Aggregated Objective does not return expected outputs using return_dict #1410

Open
arrjon opened this issue Jun 7, 2024 · 7 comments
Open
Labels
AMICI bug Something isn't working

Comments

@arrjon
Copy link
Contributor

arrjon commented Jun 7, 2024

Bug description
The functions available in pypesto.visualize.model_fit cannot work with an AggregatedObjective and hence fail.
There are two reasons:

  • the amici model is not available by objective.amici_model (I already fixed that issue in the branch visualize_aggregated by adding a new property to the AggregatedObjective)
  • calling pypesto_problem.objective(x, return_dict=True) does return empty rdatas. (this I could not fix right away)

Expected behavior
The visuslization routines should still work properly.
Also I would expect pypesto_problem.objective(x, return_dict=True) to return non empty rdatas.
Calling pypesto_problem.objective._objectives[0](x, return_dict=True) instead works and does return non empty rdatas.

To reproduce
I attached a minimal example using the Blasi model and PeTab.
minimal example.zip

Environment

  • Operating system: MacOs
  • pypesto version: branch visualize_aggregated
  • Python version: python 3.10
@arrjon arrjon added the bug Something isn't working label Jun 7, 2024
@arrjon arrjon self-assigned this Jun 7, 2024
@arrjon arrjon added the AMICI label Jun 7, 2024
@arrjon
Copy link
Contributor Author

arrjon commented Jun 7, 2024

maybe @dweindl can help here?

@dweindl
Copy link
Member

dweindl commented Jun 10, 2024

maybe @dweindl can help here?

I am not too familiar with either AggregatedObjective or pypesto.visualize.model_fit, but I agree that it should either work, or raise an informative error.

the amici model is not available by objective.amici_model

That makes sense, because often AggregatedObjective won't have only a single model (#439).

(I already fixed that issue in the branch visualize_aggregated by adding a new property to the AggregatedObjective)

I would rather handle AggregatedObjective separately in the visualization code instead of adding amici_model to AggregatedObjective, since there will probably be multiple models.

@arrjon
Copy link
Contributor Author

arrjon commented Jun 10, 2024

I would rather handle AggregatedObjective separately in the visualization code instead of adding amici_model to AggregatedObjective, since there will probably be multiple models.

Okay, I thought that AggregatedObjective is usually an AmiciObjective and NegLogPriors and not multiple models. This makes it rather complicated to write a general visualization function.

@dweindl
Copy link
Member

dweindl commented Jun 10, 2024

I don't know how it's currently used, but one of the goals would be using it with PEtab problems comprising multiple models (#439) and I wouldn't add anything that will complicate this.

I think it's nevertheless possible to provide some meaningful visualizations. Just iterate over the objectives and visualize what's possible and give a warning if any of those cannot be handled.

@arrjon arrjon removed their assignment Jun 11, 2024
@PaulJonasJost
Copy link
Collaborator

@arrjon can this be closed with the merge of #1411?

@arrjon
Copy link
Contributor Author

arrjon commented Jul 25, 2024

@arrjon can this be closed with the merge of #1411?

Regarding visualisation the bug is fixed. However, pypesto_problem.objective(x, return_dict=True) does still return empty rdatas when using an aggregated objective, which is not the behaviour I would expect.

@PaulJonasJost
Copy link
Collaborator

True, thanks then i will leave it open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMICI bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants