Add equinox module import support for petab sciml problems#3179
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3179 +/- ##
=======================================
Coverage 77.41% 77.42%
=======================================
Files 317 317
Lines 20897 20903 +6
Branches 1483 1483
=======================================
+ Hits 16178 16184 +6
Misses 4711 4711
Partials 8 8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
BSnelling
force-pushed
the
bes/equinox_petab_import
branch
from
88fe5b6 to
924be4c
Compare
| strategy: | ||
| matrix: | ||
| python-version: [ "3.11" ] | ||
| python-version: [ "3.12" ] |
There was a problem hiding this comment.
The reasoning for this is that petab-sciml v0.1 will most likely specify python >=3.12 due to numpy requirements: PEtab-dev/petab_sciml#83
Is there a specific requirement to continue 3.11 support in AMICI? From another dependency for example?
There was a problem hiding this comment.
Is there a specific requirement to continue 3.11 support in AMICI? From another dependency for example?
No, there isn't. It's time to update amici's python requirement, I'll do that on a separate PR.
BSnelling
force-pushed
the
bes/equinox_petab_import
branch
from
924be4c to
5918a04
Compare
| if isinstance(nn_model, Path): | ||
| copyfile(nn_model, filename) | ||
| return | ||
|
|
There was a problem hiding this comment.
add Path to type annotation and add processing when nn_model is string? Do we want to refuse overwriting unless something like force=True is specified?
There was a problem hiding this comment.
Added string handling and annotation.
On the overwriting, I think it is sufficient that the output directory is specified as a required arg. If we added a force keyword we'd run into a "pass-through keyword" trap. We have the force_import keyword in PetabImporter.import_module, which calls SbmlImporter and ODEExporter and finally this function generate_equinox. The kwarg would just get funnelled through all those classes.
| hybridization["net1"]["frozen_layers"], | ||
| ) | ||
|
|
||
| assert os.path.isfile(pi.output_dir / "net1.py") |
There was a problem hiding this comment.
would be nice to add some regression tests against model outputs here to also make sure the model can be called
BSnelling
force-pushed
the
bes/equinox_petab_import
branch
from
c27db5e to
020c763
Compare
3 participants
No description provided.