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

Remove check in check_error! #670

Merged
merged 3 commits into from
Apr 29, 2024
Merged

Remove check in check_error! #670

merged 3 commits into from
Apr 29, 2024

Conversation

visr
Copy link
Contributor

@visr visr commented Apr 15, 2024

Fixes #669.

I'm not sure why this check was there. Was it just to save work? Why should the postamble not run if it is successful?

Fixes SciML#669.

I'm not sure why this check was there. Was it just to save work? Why should the postamble not run if it is successful?
@ChrisRackauckas
Copy link
Member

Does this allocation get elided?

@visr
Copy link
Contributor Author

visr commented Apr 18, 2024

Running this on the example from #669 I don't see any allocations, nor do I see it in a profile.

@btime SciMLBase.check_error!($integrator);
9.510 ns (0 allocations: 0 bytes)

I decided to give AllocCheck a try. It just triggered this which looks unrelated:

using AllocCheck
@check_allocs g(i, code) = SciMLBase.solution_new_retcode(i.sol, code)
g(integrator, ReturnCode.Default)

ERROR: @check_allocs function encountered 1 errors (1 allocations / 0 dynamic dispatches).

Allocating runtime call to "jl_get_pgcstack" in unknown location

Running AllocCheck on check_error! does give some allocations from postamble! calling resize!(integrator.sol.t, integrator.saveiter).

@ChrisRackauckas
Copy link
Member

I mean in the context of a normal solve. Elision should happen in isolation but I want to make sure the compiler doesn't hit some limit say on Lorenz.

@visr
Copy link
Contributor Author

visr commented Apr 26, 2024

I saw allocations coming from postamble!. Since that isn't what I'm interested in, I put that back in the if block.

Now the number of allocations for Lorentz is all from setup, just like in this example:

https://docs.sciml.ai/DiffEqDocs/stable/tutorials/faster_ode_example/#Example-Accelerating-a-Non-Stiff-Equation:-The-Lorenz-Equation

Comment on lines +71 to +75
@test integrator.sol.retcode == ReturnCode.Default
@test check_error(integrator) == ReturnCode.Success
@test integrator.sol.retcode == ReturnCode.Default
@test SciMLBase.check_error!(integrator) == ReturnCode.Success
@test integrator.sol.retcode == ReturnCode.Success
Copy link
Member

Choose a reason for hiding this comment

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

Add a test that nothing is allocated and this is good to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is 14f6bce ok? Or do you also want to test this on Lorentz using OrdinaryDiffEq?

@ChrisRackauckas
Copy link
Member

Sorry for the delayed review, but add an allocs test and this is good.

@ChrisRackauckas
Copy link
Member

That should be sufficient.

Copy link

codecov bot commented Apr 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 31.74%. Comparing base (d947bed) to head (14f6bce).
Report is 25 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #670      +/-   ##
==========================================
- Coverage   40.55%   31.74%   -8.82%     
==========================================
  Files          55       55              
  Lines        4478     4505      +27     
==========================================
- Hits         1816     1430     -386     
- Misses       2662     3075     +413     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChrisRackauckas ChrisRackauckas merged commit 9e92fc6 into SciML:master Apr 29, 2024
27 of 42 checks passed
@visr visr deleted the retcode branch April 29, 2024 13:15
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.

check_error! returns Success but doesn't set it in sol.retcode
2 participants