-
Notifications
You must be signed in to change notification settings - Fork 4
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
Removal of dependency on SciMLBase.ODESolution
#33
Conversation
Nice! Looking forward to that. For me it is not yet fully clear which parts come from where, and whether we would still have a nice |
Examples and API shouldn't have to change much or even at all. This is mostly an internal rework. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #33 +/- ##
==========================================
- Coverage 95.12% 92.59% -2.53%
==========================================
Files 8 8
Lines 328 486 +158
==========================================
+ Hits 312 450 +138
- Misses 16 36 +20 ☔ View full report in Codecov by Sentry. |
Does this make #28 obsolete by the way? |
Yes, completely. |
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.
Sorry for quite a few comments. This looks like a very good step in the right direction!
Ah and we could update the readme, since we no longer directly depend on OrdinaryDiffEq after this PR. |
Co-authored-by: Ronny Bergmann <[email protected]>
We still depend on |
I did not check that detailed, then it can stay as is for now I think. I feel the dependency is now a bit more lightweight, but that maybe has to be very carefully phrased, since we still aim to fit into the framework. |
The dependency is indeed more lightweight. After this PR it's mostly timestepping management and the fact that we follow the same interface. |
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.
Well, I trust you that you will update the changelog and care for a bit of code coverage :)
I've created the changelog, I'm not sure why the check is failing -- probably because there is no changelog on the main branch? Code coverage is more or less as good as it can get without testing things we weren't testing before so I don't really care that much. |
Ah, news file was in the wrong folder. |
This will be the proper resolution of issues caused by abusing internals of OrdinaryDiffEq.jl. We will still use parts of SciML that make sense to keep using but there are parts we can't use such as
SciMLBase.ODESolution
.TODO:
solve
that are necessary.