Add conditions to analytic integrators#93
Conversation
clinssen
force-pushed
the
conditional-propagators
branch
from
827de32 to
bd883ac
Compare
clinssen
force-pushed
the
conditional-propagators
branch
from
1020b1b to
74ebe4d
Compare
pnbabu
left a comment
pnbabu
left a comment
There was a problem hiding this comment.
Thanks for this feature! I have a few minor comments.
| assert type(expr) is str | ||
|
|
||
| if global_dict: | ||
| global_dict = global_dict.copy() |
There was a problem hiding this comment.
Why copy the dict to the same variable?
There was a problem hiding this comment.
sympy will internally sneakily add items to this dict when it's passed to parse_expr().
There was a problem hiding this comment.
But you pass the same dict to sympy parse on L45, correct?
There was a problem hiding this comment.
Thank you for catching this! I have added clearer naming (global_dict_copy) and made an extra copy for the initial and second pass.
| initial_parse = sympy.parsing.sympy_parser.parse_expr(expr, global_dict=global_dict, local_dict=local_dict, evaluate=evaluate) | ||
| global_dict_copy = global_dict.copy() | ||
|
|
||
| initial_parse = sympy.parsing.sympy_parser.parse_expr(expr, global_dict=global_dict_copy, local_dict=local_dict, evaluate=evaluate) |
There was a problem hiding this comment.
Wouldn't this fail if global_dict is not present and therefore global_dict_copy is unassigned?
There was a problem hiding this comment.
whoops, sorry about that—fixed now!
pnbabu
left a comment
pnbabu
left a comment
There was a problem hiding this comment.
Looks good me, thank you!
|
Thank you very much for the review! |
2 participants
The current ODE-toolbox release detects conditions under which singularities can occur. This PR adds a feature that allows ODE-toolbox to generate separate solvers for each of these conditions.
Please see the rendered documentation at: https://ode-toolbox-sandbox.readthedocs.io/en/latest
For an overview of changes to the documentation, please see the diff of the file
index.rst: https://github.com/nest/ode-toolbox/pull/93/files#diff-3161f66a61b9ed68303607f1dbdce1c7bdcc69c42f8ea1c2051a07a579ebcf5dN.B. after merging this, the version number of ODE-toolbox should be bumped to 3.0.0, as this feature changes the external API.
Notes for reviewers:
real=Trueto the sympy.Symbol constructor.requirements-testing.txt.test_analytic_solver_integration.pyand introduces numerical errors, so I've set the test to skip for older versions and added a line into the documentation.Replaces #89.