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

FBC: No 'reaction2' in final version of FBC v3 #403

Closed
luciansmith opened this issue Jan 18, 2024 · 10 comments · Fixed by #406
Closed

FBC: No 'reaction2' in final version of FBC v3 #403

luciansmith opened this issue Jan 18, 2024 · 10 comments · Fixed by #406

Comments

@luciansmith
Copy link
Member

luciansmith commented Jan 18, 2024

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.

@fbergmann
Copy link
Member

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 R1^2 but sometimes an R1*R2 is needed. This is supported by cplex and gurobi, so it made sense to allow for it in the proposal. It really should be in the spec by now though.

But yeah we should not remove it from libSBML, and it should be in the spec.

@luciansmith luciansmith transferred this issue from sbmlteam/libsbml Jan 18, 2024
@luciansmith
Copy link
Member Author

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?

@fbergmann
Copy link
Member

type would still be quadratic if reaction2 is defined it would just not mean reaction1^2. So we'd only ever have reaction1*reaction2, or if no reaction2, then reaction1^2.

@bgoli
Copy link
Member

bgoli commented Jan 19, 2024

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

  • variableType linear can only be: <coef><var1>^1
  • variableType quadratic can only be: <coef><var1>^2 or <coef><var1>^1<var2>^1

which sort of represents the math expression.

The alternative would be to allow

  • variableType quadratic can only be: <coef><var1>^1<var2>^1 or <coef><var1>^1<var1>^1

where var1^2 is always represented as <var1>^1<var1>^1

The text I have currently is as follows

The required variableType attribute contains a FbcVariableType that represents the type of exponent contained by the FluxObjective. For example, where J represents a steady-state flux in the reaction attribute, the FbcVariableType allows the definition of either a “linear”, J^1 or “quadratic”, J^2 variable. In addition, a mixed “quadratic” component can be defined using the optional attribute reaction2. For example, with reaction2 represented by JR2 and a coefficient, C, a mixed two variable “quadratic” FluxObjective of the form C × J^1 × JR2^1 can be defined.

@luciansmith
Copy link
Member Author

luciansmith commented Jan 19, 2024

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.)

@bgoli
Copy link
Member

bgoli commented Feb 7, 2024

Text and UML updated to include the above in merged from #404

@bgoli bgoli linked a pull request Feb 8, 2024 that will close this issue
@luciansmith
Copy link
Member Author

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'.

@luciansmith
Copy link
Member Author

Fixed with e7d7a60

@fbergmann
Copy link
Member

@fbergmann will Deviser give us a new set of validation rules, for this and any other changes we might have missed?

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

Also, need new fbc-20650 (or something) to say that if reaction2 is defined, variableType must be 'quadratic'.

that is not something deviser could generate.

@luciansmith
Copy link
Member Author

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.

luciansmith added a commit that referenced this issue Jun 14, 2024
Fix for #403 in correct branch this time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants