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

Add strip_solution #762

Merged
merged 4 commits into from
Aug 17, 2024
Merged

Conversation

jClugstor
Copy link
Contributor

@jClugstor jClugstor commented Aug 8, 2024

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

I know error(...) is discouraged, but I couldn't find one that fit, and wasn't sure if I should make one.

This also doesn't completely strip solutions made from implicit methods, since the caches still need to be dealt with.

@jClugstor jClugstor marked this pull request as ready for review August 9, 2024 15:33
@ChrisRackauckas
Copy link
Member

Setup downstream tests

@ChrisRackauckas ChrisRackauckas merged commit 795baee into SciML:master Aug 17, 2024
31 of 43 checks passed
solution stripping.")
end

interp = strip_interpolation(sol.interp)
Copy link
Contributor

@thomvet thomvet Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jClugstor : Just randomly reviewing this. It seems strange to me that the interp variable isn't used afterwards? Should it go into line 618? Otherwise, what's the point of defining it at all? Also, maybe check that the interpolation is stripped as well in the tests if this is the intended behavior here :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that looks like a typo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a typo, thanks for pointing it out .

@ChrisRackauckas
Copy link
Member

@jClugstor what's the next steps for this?

@jClugstor
Copy link
Contributor Author

This will need to check if the solution was made using an implicit method that has caches, if it was I'll need to strip the cache. Should I make a trait for SciMLAlgs e.g. 'has_cache' ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants