feat: add Parameter and FunctionDefinition support to AntimonySerializer#319
feat: add Parameter and FunctionDefinition support to AntimonySerializer#319dyrpsf wants to merge 3 commits into
Conversation
ntung
left a comment
There was a problem hiding this comment.
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 |
|
Once again, thanks for the review, @ntung! I have pushed the updates to address your feedback:
All tests are passing locally on my end. Let me know if you need anything else! |
draeger
left a comment
There was a problem hiding this comment.
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?
1305d3a to
f1e20cd
Compare
|
Thank you for the excellent feedback, @draeger! I completely agree. Looking closely at the current Leveraging JSBML's internal abstraction (like We could implement a base abstraction layer that dynamically handles generic attribute/value pairs, and then only write custom overrides for complex components (like 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 |
|
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 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! |
|
Hi @draeger, |
Description
This PR extends the
AntimonySerializerto fully supportParameterandFunctionDefinitioncomponents.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
ParameterandFunctionDefinitionto the mainSBaserouter.toAntimony(Parameter p)andtoAntimony(FunctionDefinition fd)methods.toAntimony(Model model)loop to iterate through and append both parameter and function blocks to the final output string.ASTNodetree traversal (child nodes) was used correctly for extracting bound variables from lambda functions.Testing
AntimonySerializerTestcontinue to pass successfully.