Skip to content
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

Merged
merged 14 commits into from
Sep 3, 2024

Conversation

mateuszbaran
Copy link
Member

@mateuszbaran mateuszbaran commented Sep 2, 2024

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:

  • Adapt parts of solve that are necessary.
  • Check coverage.
  • Write changelog entry.

@mateuszbaran mateuszbaran added the preview docs Add this label if you want to see a PR-preview of the documentation label Sep 2, 2024
@kellertuer
Copy link
Member

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 solve!-tingy, but I am confident you already see a way that our examples would not have to be changed too much.

@mateuszbaran
Copy link
Member Author

Examples and API shouldn't have to change much or even at all. This is mostly an internal rework.

Copy link

codecov bot commented Sep 2, 2024

Codecov Report

Attention: Patch coverage is 86.04651% with 24 lines in your changes missing coverage. Please review.

Project coverage is 92.59%. Comparing base (af86f91) to head (7cadf45).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/ManifoldDiffEq.jl 86.33% 22 Missing ⚠️
src/interpolation.jl 50.00% 1 Missing ⚠️
src/problems.jl 50.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

src/ManifoldDiffEq.jl Outdated Show resolved Hide resolved
@mateuszbaran mateuszbaran marked this pull request as ready for review September 3, 2024 09:41
@kellertuer
Copy link
Member

Does this make #28 obsolete by the way?

@mateuszbaran
Copy link
Member Author

Yes, completely.

Copy link
Member

@kellertuer kellertuer left a 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!

src/ManifoldDiffEq.jl Outdated Show resolved Hide resolved
src/ManifoldDiffEq.jl Outdated Show resolved Hide resolved
src/ManifoldDiffEq.jl Outdated Show resolved Hide resolved
src/ManifoldDiffEq.jl Outdated Show resolved Hide resolved
src/ManifoldDiffEq.jl Outdated Show resolved Hide resolved
src/ManifoldDiffEq.jl Show resolved Hide resolved
src/ManifoldDiffEq.jl Show resolved Hide resolved
src/frozen_solvers.jl Show resolved Hide resolved
src/frozen_solvers.jl Show resolved Hide resolved
src/interpolation.jl Outdated Show resolved Hide resolved
@kellertuer
Copy link
Member

Ah and we could update the readme, since we no longer directly depend on OrdinaryDiffEq after this PR.

@mateuszbaran
Copy link
Member Author

Ah and we could update the readme, since we no longer directly depend on OrdinaryDiffEq after this PR.

We still depend on OrdinaryDiffEqCore.jl for timestepping and OrdinaryDiffEqCore.jl lives in the OrdinaryDiffEq repository so the readme isn't technically wrong here? Anyway, feel free to rephrase it if you like.

@kellertuer
Copy link
Member

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.

@mateuszbaran
Copy link
Member Author

The dependency is indeed more lightweight. After this PR it's mostly timestepping management and the fact that we follow the same interface.

Copy link
Member

@kellertuer kellertuer left a 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 :)

@mateuszbaran
Copy link
Member Author

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.

@mateuszbaran mateuszbaran merged commit 48a0cf1 into main Sep 3, 2024
10 of 13 checks passed
@mateuszbaran
Copy link
Member Author

Ah, news file was in the wrong folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview docs Add this label if you want to see a PR-preview of the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants