Support sympy 1.11 to 1.14 - #306
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates chaste_codegen to work with newer SymPy releases by broadening dependency constraints, adapting placeholder math-function substitution for SymPy’s stricter sympification behavior, and updating tests/reference outputs to support per-SymPy-version “golden” files.
Changes:
- Updated
setup.pydependency minimums/maximums (including SymPy, Pint, NetworkX, etc.) to target newer ecosystems. - Added SymPy-version-aware reference selection and updated Jacobian tests to use it, alongside new
--sympy_X_Yreference variants. - Regenerated/adjusted stored reference models and removed legacy Python 3.6 reference artifacts.
Reviewed changes
Copilot reviewed 20 out of 72 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.py | Bumps runtime dependency constraints to support newer SymPy and related packages. |
| docs/index.rst | Updates contributor instructions for dependency bumps and versioned reference files. |
| dev-requirements/dev.txt | Updates pinned dev dependency versions (notably Pint). |
| chaste_codegen/tests/test_jacobian.py | Switches Jacobian tests to SymPy-versioned reference reading. |
| chaste_codegen/tests/conftest.py | Adds SymPy-versioned reference selection and optional reference regeneration mode. |
| chaste_codegen/model_with_conversions.py | Uses subs_math_func_placeholders to avoid SymPy subs sympification issues. |
| chaste_codegen/_math_functions.py | Documents correct placeholder substitution approach for SymPy >= 1.12. |
| chaste_codegen/data/tests/test_jacobian_matrix_2.txt_python36 | Removes legacy Python 3.6-specific expected output. |
| chaste_codegen/data/tests/test_jacobian_matrix_2.txt_alt | Removes alternate legacy expected output format. |
| chaste_codegen/data/tests/test_jacobian_matrix_2--sympy_1_13.txt | Adds SymPy 1.13-specific Jacobian matrix expected output. |
| chaste_codegen/data/tests/test_jacobian_matrix_2--sympy_1_11.txt | Adds SymPy 1.11-specific Jacobian matrix expected output. |
| chaste_codegen/data/tests/test_jacobian_matrix_1.txt_python36 | Removes legacy Python 3.6-specific expected output. |
| chaste_codegen/data/tests/test_jacobian_matrix_1.txt_alt | Removes alternate legacy expected output format. |
| chaste_codegen/data/tests/test_jacobian_matrix_1--sympy_1_11.txt | Adds SymPy 1.11-specific Jacobian matrix expected output. |
| chaste_codegen/data/tests/test_jacobian_equations_2.txt_python36 | Removes legacy Python 3.6-specific expected output. |
| chaste_codegen/data/tests/test_jacobian_equations_2.txt_alt | Removes alternate legacy expected output format. |
| chaste_codegen/data/tests/test_jacobian_equations_2.txt | Updates baseline expected output to match newer SymPy behavior. |
| chaste_codegen/data/tests/test_jacobian_equations_1.txt_python36 | Removes legacy Python 3.6-specific expected output. |
| chaste_codegen/data/tests/test_jacobian_equations_1.txt_alt | Removes alternate legacy expected output format. |
| chaste_codegen/data/tests/test_jacobian_equations_1.txt | Updates baseline expected output to match newer SymPy behavior. |
| chaste_codegen/data/tests/chaste_reference_models/RLopt/hodgkin_huxley_squid_axon_model_1952_modified--sympy_1_13.cpp | Updates generated reference output for SymPy 1.13 behavior. |
| chaste_codegen/data/tests/chaste_reference_models/RL/hodgkin_huxley_squid_axon_model_1952_modified--sympy_1_13.cpp | Adds generated reference output for SymPy 1.13 (RL model type). |
| chaste_codegen/data/tests/chaste_reference_models/Opt/test_luo_rudy_1991_with_range_cap_dimensionless.cpp_python36 | Removes legacy Python 3.6 generated reference output. |
| chaste_codegen/data/tests/chaste_reference_models/Opt/hodgkin_huxley_squid_axon_model_1952_modified--sympy_1_13.cpp | Updates generated reference output for SymPy 1.13 behavior. |
| chaste_codegen/data/tests/chaste_reference_models/Opt/beeler_reuter_model_1977_lookup_tables.cpp_python36 | Removes legacy Python 3.6 generated reference output. |
| chaste_codegen/data/tests/chaste_reference_models/GRL2Opt/hodgkin_huxley_squid_axon_model_1952_modified--sympy_1_13.cpp | Updates generated reference output for SymPy 1.13 behavior. |
| chaste_codegen/data/tests/chaste_reference_models/GRL2/hodgkin_huxley_squid_axon_model_1952_modified--sympy_1_13.cpp | Updates generated reference output for SymPy 1.13 behavior. |
| chaste_codegen/data/tests/chaste_reference_models/GRL1Opt/beeler_reuter_model_1977--sympy_1_11.cpp | Updates generated reference output for SymPy 1.11 behavior. |
| chaste_codegen/data/tests/chaste_reference_models/GRL1/hodgkin_huxley_squid_axon_model_1952_modified--sympy_1_13.cpp | Updates generated reference output for SymPy 1.13 behavior. |
| chaste_codegen/data/tests/chaste_reference_models/Cvode_with_jacobian/hodgkin_huxley_squid_axon_model_1952_modified--sympy_1_13.cpp | Adds generated reference output for SymPy 1.13 (Cvode + Jacobian). |
| chaste_codegen/data/tests/chaste_reference_models/Cvode_with_jacobian/hodgkin_huxley_squid_axon_model_1952_modified--sympy_1_11.cpp | Updates generated reference output for SymPy 1.11 behavior. |
| chaste_codegen/data/tests/chaste_reference_models/Cvode_opt_with_jacobian/hodgkin_huxley_squid_axon_model_1952_modified--sympy_1_13.cpp | Adds generated reference output for SymPy 1.13 (Cvode opt + Jacobian). |
| chaste_codegen/data/tests/chaste_reference_models/Cvode_opt_with_jacobian/hodgkin_huxley_squid_axon_model_1952_modified--sympy_1_11.cpp | Updates generated reference output for SymPy 1.11 behavior. |
| chaste_codegen/data/tests/chaste_reference_models/CVODE_DATA_CLAMP_OPT/hodgkin_huxley_squid_axon_model_1952_modified--sympy_1_13.cpp | Updates generated reference output for SymPy 1.13 behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…fected variants
…kup-table and linearity analysis
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #306 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 30 30
Lines 1789 1789
=========================================
Hits 1789 1789 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…variants for ionic term ordering
…ts to python_3_11
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
MichaelClerx
left a comment
There was a problem hiding this comment.
Quick look at the first model files shows what looks like big differences in code.
Are these significant? And is this the best way to test if we have to manually review 100 files?
| const double var_background_currents__E_Ca = 0.5 * var_membrane__R * var_membrane__T * log(mParameters[1] / var_chaste_interface__intracellular_ion_concentrations__Ca_i) / var_membrane__F; // millivolt | ||
| const double var_Ca_independent_transient_outward_K_current__i_sus = (((var_environment__CT == 1) && (var_environment__PM == 0)) ? (0.098000000000000004 + 0.0014 * var_chaste_interface__membrane__V) : (((var_environment__CT == 0) && (var_environment__PM == 1)) ? (0.16799999999999998 + 0.0023999999999999998 * var_chaste_interface__membrane__V) : (0.070000000000000007 + 0.001 * var_chaste_interface__membrane__V))); // picoA | ||
| const double var_Ca_independent_transient_outward_K_current__i_to = (((var_environment__CT == 1) && (var_environment__PM == 0)) ? (0.20000000000000001 * (0.40000000000000002 + 0.59999999999999998 * pow(var_chaste_interface__Ca_independent_transient_outward_K_current_s3_gate__s3, 6)) * (-var_Ca_independent_transient_outward_K_current__E_K + var_chaste_interface__membrane__V) * (0.58999999999999997 * pow(var_chaste_interface__Ca_independent_transient_outward_K_current_s1_gate__s1, 3) + 0.40999999999999998 * pow(var_chaste_interface__Ca_independent_transient_outward_K_current_s2_gate__s2, 3)) * var_Ca_independent_transient_outward_K_current__g_to * var_chaste_interface__Ca_independent_transient_outward_K_current_r_gate__r) : (((var_environment__CT == 0) && (var_environment__PM == 1)) ? (0.34999999999999998 * (0.40000000000000002 + 0.59999999999999998 * pow(var_chaste_interface__Ca_independent_transient_outward_K_current_s3_gate__s3, 6)) * (-var_Ca_independent_transient_outward_K_current__E_K + var_chaste_interface__membrane__V) * (0.58999999999999997 * pow(var_chaste_interface__Ca_independent_transient_outward_K_current_s1_gate__s1, 3) + 0.40999999999999998 * pow(var_chaste_interface__Ca_independent_transient_outward_K_current_s2_gate__s2, 3)) * var_Ca_independent_transient_outward_K_current__g_to * var_chaste_interface__Ca_independent_transient_outward_K_current_r_gate__r) : ((0.40000000000000002 + 0.59999999999999998 * pow(var_chaste_interface__Ca_independent_transient_outward_K_current_s3_gate__s3, 6)) * (-var_Ca_independent_transient_outward_K_current__E_K + var_chaste_interface__membrane__V) * (0.58999999999999997 * pow(var_chaste_interface__Ca_independent_transient_outward_K_current_s1_gate__s1, 3) + 0.40999999999999998 * pow(var_chaste_interface__Ca_independent_transient_outward_K_current_s2_gate__s2, 3)) * var_Ca_independent_transient_outward_K_current__g_to * var_chaste_interface__Ca_independent_transient_outward_K_current_r_gate__r))); // picoA | ||
| const double var_Ca_independent_transient_outward_K_current__i_sus = (((var_environment__CT == 1) && (var_environment__PM == 0)) ? (0.098000000000000004 + 0.0014 * var_chaste_interface__membrane__V) : (((var_environment__PM == 1) && (var_environment__CT == 0)) ? (0.16799999999999998 + 0.0023999999999999998 * var_chaste_interface__membrane__V) : (0.070000000000000007 + 0.001 * var_chaste_interface__membrane__V))); // picoA |
There was a problem hiding this comment.
Is this just pseudo-random differences? Or were we e.g. sorting before but not now? Would be nice to avoid reviewing a 100 models 😅
There was a problem hiding this comment.
Good point! Sorry about the number of new reference files. In this particular one, it looks like I've managed to confuse git into thinking an old python 3.6 alternative reference (which has been deleted) was renamed to a newly added alternative reference generated with sympy 1.14.
It might be worth adding in some numerical tests to check things.
There was a problem hiding this comment.
This ordering is pre-existing between the python 3.6 variant and the original reference as well.
| const double var_x10 = 1 / (1.3999999999999999 + var_x9 + 0.80000000000000004 * var_chaste_interface__Ca_handling_by_the_SR__Ca_up); | ||
| const double var_x11 = (var_x9 - 0.32000000000000006 * var_chaste_interface__Ca_handling_by_the_SR__Ca_up) / pow((0.00041999999999999996 + 0.00023999999999999998 * var_chaste_interface__Ca_handling_by_the_SR__Ca_up + var_chaste_interface__intracellular_ion_concentrations__Ca_i), 2); | ||
| const double var_x12 = 0.23999999999999999 * exp(-1.6000000000000001 + 0.080000000000000002 * var_chaste_interface__membrane__V); | ||
| const double var_x8 = 1.3999999999999999 + 0.80000000000000004 * var_chaste_interface__Ca_handling_by_the_SR__Ca_up + 3333.3333333333335 * var_chaste_interface__intracellular_ion_concentrations__Ca_i; |
There was a problem hiding this comment.
These look like real differences. What's going on?
There was a problem hiding this comment.
This is also pre-existing between Python 3.6 and the main reference, although it's a bit different there as well with:
const double var_x8 = 1 / (1.3999999999999999 + 0.80000000000000004 * var_chaste_interface__Ca_handling_by_the_SR__Ca_up + 3333.3333333333335 * var_chaste_interface__intracellular_ion_concentrations__Ca_i);Probably needs numerical testing.
There was a problem hiding this comment.
Might just be re-organising the temporary variables to represent different bits of calculation for some reason. I think the numerical tests are done within Chaste, so could try and run it with this branch if that's possible?
There was a problem hiding this comment.
| rJacobian[1][13] = 0.0; | ||
| rJacobian[2][0] = 0.0; | ||
| rJacobian[2][1] = 0.0; | ||
| rJacobian[2][2] = 1.0 - (mDt * (-var_x12 - var_x17)); |
There was a problem hiding this comment.
What's happening here?
There was a problem hiding this comment.
This also seems to be pre-existing between the Python 3.6 version and the main reference, although the original main reference has something slightly different:
rJacobian[2][2] = 1.0 - (mDt * (-var_x16));I think these may be due to a stale Python 3.6 alternative version, but definitely worth putting in some numerical testing!
Thanks @MichaelClerx . These particular changes are pre-existing and don't appear to be significant. I am getting all the generated reference models to run through numerical testing, so we probably don't need to review all of those manually. |
Description
Add support for newer versions of sympy
expr.replace()instead ofexpr.subs()where appropriate due tosubsstrictly requiring Sympy objects in 1.12+._altand_python36reference code variants.sympy_1_11variants due to coefficient folding indiff.sympy_1_13variants due to extracting negativeFloatexponents inAdd.sympy_1_14variants due to disablingFloatexponent extractions inAdd.python_3_11variants due to a different ionic current sum ordering in Python3.10 + Pint 0.24, resulting from aFloat(1.0)passing into coefficients e.g.1.0 * HeartConfig::Instance()->GetCapacitance()in_get_ionic_vars. The1.0disappears in a different Python or Pint version.Note
The added
regenerate_reference(..)inconftest.pycurrently piggy-backs on the tests to generate reference files. It could possibly be moved into a separate "infra scripts" folder, but it might not be worth creating an infra section for just the one case. Also, it automatically labels variants with the sympy version used to generate them as this causes the majority of differences, so some manual renaming may be needed if e.g. the Python version is the cause of the difference.Motivation and Context
Fixes #285
Types of changes
Documentation checklist
Testing