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

Improve the efficiency of the AssignmentCycles::determineAllDependencies() function #374

Closed
eltix opened this issue May 17, 2024 · 6 comments · Fixed by #377
Closed

Improve the efficiency of the AssignmentCycles::determineAllDependencies() function #374

eltix opened this issue May 17, 2024 · 6 comments · Fixed by #377

Comments

@eltix
Copy link
Contributor

eltix commented May 17, 2024

Hi all,

The AssignmentCycles::determineAllDependencies() function does not behave well with models having many parameters and rules and transitive dependencies between model constructs.
Unfortunately, I cannot share any of the actual models we have had issues with for they are confidential. However, I have crafted a couple of examples (see attachments) which exhibit somewhat similar structures.

  • The smallest example Model150.xml has 150 parameters and 147 assignment rules and the determineAllDependencies step takes around 1min on a regular machine.
  • The Model180.xml one has 180 parameters and 177 assignment rules, it takes 3min.
  • The largest one Model300.xml takes forever to terminate

For our actual models, this function takes a prohibitive time (> 1hour) to terminate even though the ODE system solving part takes less than a minute. As a consequence, we have temporarily bypassed the consistency checks altogether in our pipelines.

I would gladly get involved myself in the search for a suitable solution but my C++ skills are alas quite limited.
Thank you all for maintaining this library!

Attachments

determineAllDependenciesInputs.zip

Contains:

  1. the "small" Model150.xml example
  2. the "medium" Model180.xml example
  3. the "large" Model300.xml example
  4. A runConsistencyChecks.cpp file to load a document and run the consistency checks
eltix added a commit to eltix/libsbml that referenced this issue May 21, 2024
Very slow for large models having transitive dependencies, see sbmlteam#374
@fbergmann
Copy link
Member

Without a model it will be really hard for us to determine whether we managed to tackle the performance issue. Could you perhaps run the validation once (overnight) for your 150 /180 model with valgrind:

valgrind --tool=callgrind ./<executable that validates your model>

the validate c++ example would work fine for that.

thank you

@eltix
Copy link
Contributor Author

eltix commented May 24, 2024

Thank you for your answer @fbergmann
I don't really understand what you mean by "Without a model"? All three models are in the zip bundle I attached to the issue description.

@fbergmann
Copy link
Member

indeed, nevermind. running cachegrind now to see where this is coming from

@fbergmann
Copy link
Member

maybe you can try the code from the branch, with that the model300 completes in a couple of seconds for me

@eltix
Copy link
Contributor Author

eltix commented May 27, 2024

Thank you @fbergmann, I confirm your fix drastically improves the performance. For our problematic model, we went from close to infinite time to 2min30s which is already a great improvement.

@fbergmann
Copy link
Member

You might want to think about encoding your model in SBML L2V1, there the restriction for assignment cycles does not hold, so the test would be skipped altogether. Since your model seems to use no features from L3V2 it might be something worth considering.

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 a pull request may close this issue.

2 participants