Support initialAssignments with the DefaultImporter #132
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #132 +/- ##
==========================================
+ Coverage 93.94% 94.24% +0.29%
==========================================
Files 7 7
Lines 347 365 +18
==========================================
+ Hits 326 344 +18
Misses 21 21
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
![ai-maintainer[bot]](https://avatars.githubusercontent.com/in/339956?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI-Maintainer Review for PR - Pl/initial assignments
Title and Description 👍
The description of the pull request provides a clear explanation of the purpose of the changes. It effectively communicates the intent to support `initialAssignments` for users who do not want to flatten them during libSBML conversion. However, the title of the pull request could be more descriptive to better convey the purpose of the changes.
Scope of Changes 👍
The changes in the pull request are narrowly focused on addressing a specific issue related to `initialAssignments` during libSBML conversion. The modifications primarily involve adding and updating code related to `u0map`, `parammap`, and `initial_assignment_map` in the `Catalyst.ReactionSystem` function and the `get_mappings` function. There are also some additions and modifications in the `create_symbol` function. The changes are focused on resolving the specific issue at hand rather than addressing multiple issues simultaneously.
Documentation 👎
The newly added function `create_symbol` in both `src/utils.jl` and `test/utils.jl` does not have docstrings. It would be beneficial to add docstrings that describe the behavior, arguments, and return values of these functions.
Testing 👎
The description does not provide information on how the changes were tested. It would be helpful to include details about the testing process, such as the specific test cases used or any additional steps taken to ensure the correctness and functionality of the modifications.
Suggested Changes
- Add a more descriptive title for the pull request that better conveys the purpose of the changes.
- Add docstrings to the
create_symbolfunction in bothsrc/utils.jlandtest/utils.jlthat describe the behavior, arguments, and return values of the function. - Include information in the description about how the changes were tested. This could include specific test cases used or additional steps taken to ensure the correctness and functionality of the modifications.
Reviewed with AI Maintainer
src/systems.jl
Outdated
| for (k, v) in model.initial_assignments | ||
| var = create_symbol(k, model) | ||
| val = model.initial_assignments[k] | ||
| push!(initial_assignment_map, var => interpret_as_num(val, model)) # Todo |
There was a problem hiding this comment.
What's the todo here about?
There was a problem hiding this comment.
It is already done, but I forgot to delete the comment.🙈
src/systems.jl
Outdated
|
|
||
| for (k, v) in model.initial_assignments | ||
| var = create_symbol(k, model) | ||
| val = model.initial_assignments[k] |
There was a problem hiding this comment.
What's the difference between v and val?
There was a problem hiding this comment.
None. Well spotted. Changed that.
paulflang
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is to support initialAssignments for users who do not want to flatten them during libSBML conversion (i.e. with the
import_simplify_math) converter from SBML.jl. As theDefaultImporterno longer uses this converter, it is needed for correct default imports.Documentation: I did not add a docstring to the new function
create_symbolas it is not exported.Testing: I wanted to add a test function for
create_symbolbut this requires SciML/ModelingToolkit.jl#2228.