Skip to content

Port improved comp flattening from eikept/jsbml and run tests#277

Open
dyrpsf wants to merge 4 commits intosbmlteam:masterfrom
dyrpsf:improve-flattening-tests
Open

Port improved comp flattening from eikept/jsbml and run tests#277
dyrpsf wants to merge 4 commits intosbmlteam:masterfrom
dyrpsf:improve-flattening-tests

Conversation

@dyrpsf
Copy link
Contributor

@dyrpsf dyrpsf commented Jan 16, 2026

This PR addresses part of #251 by porting the improved comp flattening implementation
from the eikept/jsbml fork and wiring it into the current jsbml-comp module.

Main changes

  • Replaced CompFlatteningConverter with the version from the eikept/jsbml fork,
    which provides a more complete flattening implementation for the comp package
    (including proper handling of ReplacedElement/Deletion chains, conversion factors,
    and nested submodels).
  • Added the helper classes used by the new converter:
    • CompFlatteningIDExpander
    • CompFlatteningIDExpanderTS
  • Adjusted the mergeModels method to merge the “current” (parent) model before
    the previously-flattened submodels. This preserves the original parameter ordering,
    so that flattened output matches the existing testFlattening/testN_flat.xml
    expectations (e.g. for test6).

Testing

From this branch I ran:

mvn -pl :jsbml-comp -Dtest=CompFlattenTest#testSpecificFile test
mvn -pl :jsbml-comp -Dtest=CompFlattenTest test

Results:

  • testSpecificFile and the individual flattening/internalisation tests now pass for the local testFlattening and testGathering data.

  • testAllData now exercises the full testFlattening suite; there is still one mismatch reported via Success Testing Model (this was previously flagged in the test with a TODO comment).

  • testInternaliseExternalModelDefinitions_online still errors due to the remote URL (https://sbmlteam.github.io/jsbml/test/data/comp/simple_online_url.xml) returning a 301 HTML redirect instead of SBML; this looks like a test-data/ hosting issue rather than an algorithm problem.

I’ve kept those two behaviours unchanged but can help adjust the tests if you
prefer (e.g. updating the URL to a raw SBML resource or marking the online test
as integration-only).

@dyrpsf dyrpsf closed this Jan 24, 2026
@dyrpsf dyrpsf deleted the improve-flattening-tests branch January 24, 2026 03:28
@dyrpsf dyrpsf restored the improve-flattening-tests branch January 24, 2026 03:42
@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!

@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!

@draeger draeger self-requested a review March 11, 2026 21:27
@draeger draeger linked an issue Mar 11, 2026 that may be closed by this pull request
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.

Very good work. My remarks focus on declarations of some data types that could be more generic in a few places.

@dyrpsf
Copy link
Contributor Author

dyrpsf commented Mar 12, 2026

Hi @draeger ,

I have refactored CompFlatteningConverter.java to use generic interfaces (Map, List) for field declarations and method parameters as requested.

I noticed that OverdeterminationValidator.java and its corresponding test also use HashMap and ArrayList. Would you like me to apply the same type-level refactoring to those files as well, or should we keep this PR focused strictly on the flattening converter?

The project compiles successfully, though some existing network-related timeouts persist in TestExternalModelDefinition.

Thanks!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve flattening by testing and merging an existing fork

2 participants