-
Notifications
You must be signed in to change notification settings - Fork 3
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
FBC: No 'reaction2' in final version of FBC v3 #403
Comments
Let's bring @bgoli in on this as well. The issue that came up was that the new spec basically goes through a lot of hoops to allow for quadratic constraints. While implementing that it turned out that sometimes you don't just need an But yeah we should not remove it from libSBML, and it should be in the spec. |
OK! Transferred it to the spec repository instead, then. Question that the spec should answer: What does the 'variableType' mean if reaction2 is defined? Is it legal to have a reaction2 if the variableType is 'quadratic'? Is it possible to have R1^2R2? Or R1^2R2^2? |
type would still be |
Sorry all for the delay, I have the updated text and UML with the quadratic stuff almost ready for a PR. My thoughts are along the same lines as Frank's above: For a FluxObjective and UDCC
which sort of represents the math expression. The alternative would be to allow
where var1^2 is always represented as The text I have currently is as follows
|
That works for me. I would also add a validation rule that says if 'reaction2' is defined, the variableType must be 'quadratic'. (And make sure the existing validation rules include 'reaction2' as a valid optional attribute.) |
Text and UML updated to include the above in merged from #404 |
The validation rule fbc-20603 is still incorrect; it needs to mention that fbc:reaction2 is allowed. @fbergmann will Deviser give us a new set of validation rules, for this and any other changes we might have missed? Also, need new fbc-20650 (or something) to say that if reaction2 is defined, variableType must be 'quadratic'. |
Fixed with e7d7a60 |
Some of them probably could be generated (and were already for libSBML with commit sbmlteam/libsbml@aee04c3#diff-9c48759d21d1813414d868773d6d6a9ca71bc6a17c19d3b9de98984edd8956a2). Latest version of the deviser spec for V3 is https://github.com/sbmlteam/libsbml/blob/development/dev/packages/deviser-fbc_v3.xml
that is not something deviser could generate. |
I didn't try to regenerate the entire set of validation rules, but did update them to include reaction2, and created a new 20650 by hand. I think this is all we need for that. |
Fix for #403 in correct branch this time.
libsbml has support for a 'reaction2' attribute of a FluxObjective, but no such attribute exists in the final spec.
@fbergmann @skeating Can you confirm that the attribute wasn't mistakenly left out of the spec? If not, we should remove 'reaction2' from libsbml.
The text was updated successfully, but these errors were encountered: