Skip to content

Conversation

@dyrpsf
Copy link
Contributor

@dyrpsf dyrpsf commented Jan 14, 2026

###Summary

Fixes #138 .

In SBML Level 3 Version 2, rateOf csymbols can target a reaction identifier. The OverdeterminationValidator was not correctly taking these references into account because, in some cases, the ASTNode for the identifier has no variable set, and the identifier was therefore ignored when building the bipartite graph. This could cause overdetermined models involving rateOf(reactionId) to go undetected.

Changes

  1. OverdeterminationValidator
  • Added a helper:
private SBase resolveSBaseFromId(String id)

which resolves an identifier string to the corresponding SBase in the model (Species, Compartment, Parameter, or Reaction).

  • Updated
private void getVariables(ListOf<LocalParameter> param,
                          ASTNode node,
                          List<SBase> variables,
                          int level)

to:

First use node.getVariable() as before.
If getVariable() returns null (which can happen for identifiers used under a rateOf csymbol in L3V2), fall back to resolveSBaseFromId(node.getName()).
Preserve existing behavior for:
excluding local parameters

  • special Level 1 parameter insertion ordering

  • ignoring time and Avogadro symbols.

  • As a result, algebraic rules whose MathML uses rateOf(reactionId) now correctly link to the corresponding reaction node in the validator’s bipartite graph.

  1. Tests
  • Added core/test/org/sbml/jsbml/validator/OverdeterminationValidatorTest.java.

  • New test:

testGetVariablesResolvesReactionIdWhenVariableNotSet()
  • Builds a minimal L3V2 model with a single reaction R1.

  • Creates an ASTNode that refers to "R1" by name, without setting its variable (mimicking the situation for rateOf targets).

  • Invokes the private getVariables(...) method via reflection.

  • Asserts that the resulting variable list contains the reaction R1.

Testing

On this branch I ran:

  • mvn -pl core -am -DskipTests install

  • mvn -pl core test

Both complete with BUILD SUCCESS. No changes were made to project POMs or external dependencies.

@dyrpsf dyrpsf closed this Jan 24, 2026
@dyrpsf dyrpsf deleted the fix-overdetermination-rateOf branch January 24, 2026 03:28
@dyrpsf dyrpsf restored the fix-overdetermination-rateOf branch January 24, 2026 03:32
@dyrpsf dyrpsf deleted the fix-overdetermination-rateOf branch January 24, 2026 03:40
@dyrpsf dyrpsf restored the fix-overdetermination-rateOf branch January 24, 2026 03:40
@dyrpsf dyrpsf reopened this Jan 24, 2026
@dyrpsf dyrpsf closed this Jan 24, 2026
@dyrpsf dyrpsf force-pushed the fix-overdetermination-rateOf branch from 0ac6e59 to 22659a7 Compare January 24, 2026 04:27
@dyrpsf
Copy link
Contributor Author

dyrpsf commented Jan 24, 2026

Hi, this PR was automatically closed because I had to reset and clean up my branches while reverting recent changes.
I’ll reopen a fresh PR once the work is re-applied cleanly.
Thanks!

@draeger draeger self-assigned this Jan 24, 2026
@dyrpsf dyrpsf reopened this Jan 24, 2026
@dyrpsf
Copy link
Contributor Author

dyrpsf commented Jan 24, 2026

Hi everyone,

Just a quick update: everything is now set and all issues have been resolved. The changes have been reapplied cleanly, and this PR, as well as all other PRs I previously mentioned with problems, are now in good shape. Feel free to review at your convenience.

Thanks for your patience!

Copy link
Member

@draeger draeger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far. I'd like to see a second review.

@dyrpsf
Copy link
Contributor Author

dyrpsf commented Jan 26, 2026

Quick update (as per the suggestion from @draeger ):

  • I’ve replaced the manual getSpecies/getCompartment/getParameter/getReaction sequence with your suggested approach using
    model.findNamedSBase(id) in the helper method.

  • The helper now calls findNamedSBase(id) once and returns the result only if it is a
    Species, Compartment, Parameter, or Reaction; otherwise it returns null.

  • Rebuilt and re‑ran the core tests with Java 11:

    mvn -pl core test

    which completes with BUILD SUCCESS.
    So the behavior is the same, but the implementation is simpler and relies on the central API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

correct OverdeterminationValidator for L3V2

2 participants