opencobra / memote

memote – the genome-scale metabolic model test suite
https://memote.readthedocs.io/
Apache License 2.0
128 stars 28 forks source link

Cycles met ids #756

Closed hgscott closed 1 year ago

hgscott commented 1 year ago
Midnighter commented 1 year ago

The tests fail because I need to make a fix to update the depinfo dependency... Not for you to worry about.

hgscott commented 1 year ago

Thank you for the changes! I'm curious to see all the problems this test will have once it is actually run on all the models where it was skipped before.

Please just make the changes for catching exceptions, the rest looks good!

One thing to note that I found, because the detect_energy_generating_cycles functions returns all reactions where the flux is greater than 0.0, the list can be frighteningly long, but the vast majority of the reactions have a very low flux. So increasing the lower limit from 0.0 to something like 0.0001 decreases the number of reactions found by a lot (and probably gives a more helpful image of what is actually problematic).

I just don't want people to generate the reports and freak out that all their models are broken!

Midnighter commented 1 year ago

That's a good point. I suppose the flux should at least be greater than the tolerance of the solver.

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (5150a3a) 74.85% compared to head (18fbffe) 74.85%.

:exclamation: Current head 18fbffe differs from pull request most recent head 3e505ca. Consider uploading reports for the commit 3e505ca to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #756 +/- ## ======================================== Coverage 74.85% 74.85% ======================================== Files 50 50 Lines 2955 2955 Branches 669 669 ======================================== Hits 2212 2212 Misses 649 649 Partials 94 94 ```

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

Midnighter commented 1 year ago

One thing to note that I found, because the detect_energy_generating_cycles functions returns all reactions where the flux is greater than 0.0, the list can be frighteningly long, but the vast majority of the reactions have a very low flux. So increasing the lower limit from 0.0 to something like 0.0001 decreases the number of reactions found by a lot (and probably gives a more helpful image of what is actually problematic).

Now I merged your work without taking this into account. Would you be up for creating another PR to address this?