-
Notifications
You must be signed in to change notification settings - Fork 32
Move sundials-related functionality to amici.sim.sundials.*
#3060
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f838fbd to
ce6897c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3060 +/- ##
=======================================
Coverage 78.15% 78.16%
=======================================
Files 312 313 +1
Lines 20643 20656 +13
Branches 1499 1499
=======================================
+ Hits 16133 16145 +12
- Misses 4500 4502 +2
+ Partials 10 9 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
2f1d027 to
46b74e9
Compare
| "import numpy as np" | ||
| "import numpy as np\n", | ||
| "from amici.sim.sundials import ExpData, run_simulation\n", | ||
| "from amici.sim.sundials.plotting import plot_observable_trajectories" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave in amici.plotting? Doesn‘t have to be sundial specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, all of it relies on ExpData, amici::Model, ReturnData. We can make them more generic, but I'd find it cleaner to have separate implementations, as the involved objects are quite different.
| " SensitivityMethod,\n", | ||
| " SensitivityOrder,\n", | ||
| " get_data_observables_as_data_frame,\n", | ||
| " get_edata_from_data_frame,\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are trhese still useful? :( I had hoped we could just silently get rid of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Half of the functionality is available through the ReturnData xarray interface, but that doesn't encode the experimental conditions. Not sure how relevant.
Serialization of ExpData is meanwhile available elsewhere.
I just searched on GitHub and got 0 hits outside of amici. For this PR, I'd leave it in, but we can remove it afterwards.
-> #3063
46b74e9 to
0bad2c4
Compare
This was supposed to be removed already in AMICI-dev#3064. Looks like some merge issue with AMICI-dev#3060.
This was supposed to be removed already in AMICI-dev#3064. Looks like some merge issue with AMICI-dev#3060.
Related to #3041.