sbmlteam / sbml-specifications

The specification documents for SBML.
6 stars 2 forks source link

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

Closed luciansmith closed 3 months ago

luciansmith commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 <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 commented 8 months ago

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 commented 8 months ago

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

luciansmith commented 7 months ago

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 commented 7 months ago

Fixed with https://github.com/sbmlteam/sbml-specifications/commit/e7d7a60319cf011e8e8806585720d22291e1dfb0

fbergmann commented 7 months ago

@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 https://github.com/sbmlteam/libsbml/commit/aee04c30dee025210da7928835bd4793a6e67a50#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 commented 3 months ago

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.