-
Notifications
You must be signed in to change notification settings - Fork 218
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
Reaction.summary() broken for some models #1118
Comments
Oh I see, the problem is the filter in line 518. If the flux<threshold this will cause the key error. I don't get the logic though, what was the output supposed to be if the flux is basically zero? Shouldn't you still report the zero flux? |
The summaries have always filtered low flux. I think the default was 1e-6. But indeed, need to handle the reactions not being present any longer. |
I think that makes sense for the model and metabolite summary but for the reaction I would just show the actual value. Setting it to zero wouldn't be correct either since we don't know if it's actually zero, so just showing the estimated flux seems okay to me. |
So you wouldn't even set it to zero if the absolute value was below tolerance? I agree that it's weird to not show anything when you request the summary of a single reaction. |
Yeah, exactly. Because you don't know whether the flux is zero. Just that solver can't differentiate if the value is different from zero. An example would be if I set the reactions lower bound to 1e-7. In that case the summary would now show an infeasible flux even if the solution was feasible. |
Alternative suggestion: output 0 along with the tolerance range. So |
Problem description
Reconstructed a model for a course with CARVEME and was surprised that Reaction.summary() errors with a key error.
Code Sample
Create a minimal, complete, verifiable example.
Here is the model that causes the crash:
SRR9275468.asm1.xml.gz
Context
The text was updated successfully, but these errors were encountered: