Remove ODES solver#3932
Remove ODES solver#3932kratman merged 20 commits intopybamm-team:developfrom kratman:feat/removeODES
Conversation
|
I created this PR very early since I want to test on CI. This is in no way ready for review |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3932 +/- ##
===========================================
- Coverage 99.60% 99.58% -0.03%
===========================================
Files 259 257 -2
Lines 21347 21190 -157
===========================================
- Hits 21263 21102 -161
- Misses 84 88 +4 ☔ View full report in Codecov by Sentry. |
|
🎵 Should Old Acquaintance be forgot... 🎵 |
|
For the macOS failures, removing some of the recent |
|
Since tests seem to be passing, I am going to open this up for review Extra things for reviewers to check:
|
Saransh-cpp
left a comment
There was a problem hiding this comment.
Thanks, @kratman! This looks good!
Dockerfile should also be updated to remove the extra odes dependency. Additionally, the CHANGELOG note should ideally be placed under the "Breaking changes" section and should specify why the solver was removed.
Yeah I was making the changelog now. Good catch on the dockerfile |
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
|
On another note, we shouldn't need to install two of the macOS system dependencies now with this, just |
Can you add this to #3941 and I will look at it there |
Changes were address. Additional improvements will be done in a follow task
|
Coverage will be fixed in #3942 |
* Remove ODES * Remove some additional ODES files * More ODES removals * Update .github/workflows/run_periodic_tests.yml * Change versions and fix comments * Remove some unneeded comments * Change log and docker * Apply suggestions from code review Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com> * Docker fix and bumping version in a test * Revert test version * Replace skipped test * Update change log --------- Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
Description
Since the ODES solver was causing issues for upgrading to python 3.12 and might not offer anything outside of our other solvers, it might be able to be removed.
Key checklist:
$ pre-commit run(or$ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all(or$ nox -s tests)$ python run-tests.py --doctest(or$ nox -s doctests)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick(or$ nox -s quick).Further checks: