Skip to content

feat: add Parameter and FunctionDefinition support to AntimonySerializer#319

Open
dyrpsf wants to merge 3 commits into
sbmlteam:masterfrom
dyrpsf:feat/antimony-parameters-functions
Open

feat: add Parameter and FunctionDefinition support to AntimonySerializer#319
dyrpsf wants to merge 3 commits into
sbmlteam:masterfrom
dyrpsf:feat/antimony-parameters-functions

Conversation

@dyrpsf

@dyrpsf dyrpsf commented May 31, 2026

Copy link
Copy Markdown
Contributor

Description

This PR extends the AntimonySerializer to fully support Parameter and FunctionDefinition components.

By serializing these components into the Antimony scripting language, we ensure that the entire mathematical model—including global parameters and custom lambda functions—is readable by downstream tools and LLM pipelines.

Changes Made

  • Added Parameter and FunctionDefinition to the main SBase router.
  • Implemented specific toAntimony(Parameter p) and toAntimony(FunctionDefinition fd) methods.
  • Updated the toAntimony(Model model) loop to iterate through and append both parameter and function blocks to the final output string.
  • Ensured JSBML-specific ASTNode tree traversal (child nodes) was used correctly for extracting bound variables from lambda functions.

Testing

  • All 16 tests in AntimonySerializerTest continue to pass successfully.

@ntung ntung left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It would be great to have tests for the new implementation. We will soon set of the CI pipeline job to make sure that the tests reach a specific threshold before pushing.

Comment thread core/src/org/sbml/jsbml/util/AntimonySerializer.java Outdated
@dyrpsf

dyrpsf commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

It would be great to have tests for the new implementation. We will soon set of the CI pipeline job to make sure that the tests reach a specific threshold before pushing.

Thanks for the review! I completely agree. I will add the test cases for both Parameter and FunctionDefinition serialization to AntimonySerializerTest.java and push the updates shortly.

@dyrpsf

dyrpsf commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Once again, thanks for the review, @ntung! I have pushed the updates to address your feedback:

  • Constants: Replaced the hardcoded \nend\n string with the AntimonyConstants.END constant to maintain architectural consistency.
  • Unit Tests: Added unit tests for both Parameter and FunctionDefinition serialization in AntimonySerializerTest.java.

All tests are passing locally on my end. Let me know if you need anything else!

@draeger draeger self-requested a review June 19, 2026 10:37

@draeger draeger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very good work. I wonder whether we could make stronger use of JSBML’s existing abstraction mechanisms instead of handling many concrete classes individually.

For example, writeXMLAttributes might be one possible entry point, but this is not necessarily the only option. The general idea would be to collect attribute/value pairs or similar structural information at a higher abstraction level and then translate this into Antimony syntax where possible. For attributes or keywords whose names differ between SBML and Antimony, a simple mapping layer, such as a lookup table or properties-based configuration, might be sufficient.

Another possible source of inspiration could be the implementation of JSBML’s toString methods, which use Java reflection to avoid duplicating similar logic across many classes.

Such an approach could reduce the number of class-specific special cases and might allow some subclasses to inherit their Antimony serialization almost entirely from a superclass.

Do you think this would be feasible for at least some SBML elements?

Comment thread core/src/org/sbml/jsbml/util/AntimonySerializer.java Outdated
@dyrpsf dyrpsf force-pushed the feat/antimony-parameters-functions branch from 1305d3a to f1e20cd Compare June 19, 2026 15:09
@dyrpsf

dyrpsf commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for the excellent feedback, @draeger! I completely agree. Looking closely at the current AntimonySerializer, the toAntimony(SBase element) router and the explicit isSet...() attribute checks definitely create a lot of boilerplate.

Leveraging JSBML's internal abstraction (like writeXMLAttributes or reflection as used in toString) is a fantastic architectural idea to make this scalable. I think it would be highly feasible, especially for elements with flat attribute structures like Compartment, Parameter, and basic Species, where the translation is mostly mapping structural attributes to Antimony keywords.

We could implement a base abstraction layer that dynamically handles generic attribute/value pairs, and then only write custom overrides for complex components (like Reaction kinetic laws or Event triggers/assignments) that require specific ASTNode math formatting.

Since implementing this reflection/abstraction approach would involve a significant rewrite of the core serializer class, would you prefer I attempt to integrate it directly into this current PR, or should we merge these specific Parameter and FunctionDefinition additions first and track the architectural refactor in a separate dedicated issue?

@draeger

draeger commented Jun 19, 2026

Copy link
Copy Markdown
Member

Thank you, this sounds like a very good assessment.

I would prefer not to make this current PR too large. Since the present changes for Parameter and FunctionDefinition already provide useful functionality, I think it would be better to merge these additions first, provided that the current implementation works correctly and the tests pass.

The more generic reflection- or abstraction-based serializer design should probably be tracked in a separate dedicated issue. That would allow us to discuss the architecture, identify which SBML elements can be handled generically, and decide where custom overrides are still necessary.

So my suggestion would be: let us keep this PR focused on the current additions, and open a follow-up issue for the broader AntimonySerializer refactoring.

@draeger draeger requested review from draeger and ntung June 19, 2026 16:09
@dyrpsf

dyrpsf commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Thank you, this sounds like a very good assessment.

I would prefer not to make this current PR too large. Since the present changes for Parameter and FunctionDefinition already provide useful functionality, I think it would be better to merge these additions first, provided that the current implementation works correctly and the tests pass.

The more generic reflection- or abstraction-based serializer design should probably be tracked in a separate dedicated issue. That would allow us to discuss the architecture, identify which SBML elements can be handled generically, and decide where custom overrides are still necessary.

So my suggestion would be: let us keep this PR focused on the current additions, and open a follow-up issue for the broader AntimonySerializer refactoring.

That sounds like a perfect plan. I completely agree with keeping this PR focused, and since all tests are currently passing, this branch should be ready to go.

I will open a separate, dedicated issue shortly to track the reflection and abstraction-based refactoring. That will give us a clean slate to discuss the architecture and map out exactly which elements can be handled generically versus those that need custom overrides.

Thanks again for the excellent guidance and review!

@dyrpsf

dyrpsf commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Hi @draeger,
As we can see in this PR, Tung already approved the changes up to commit 299f512. After that, I pushed only one additional commit (f1e20cd), which I had accidentally left out earlier. Therefore, I think this PR is ready to be merged.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants