-
-
Notifications
You must be signed in to change notification settings - Fork 546
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
Remove ODES solver #3932
Remove ODES solver #3932
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:
|
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.
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>
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.
So long, old friend
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: