search

SciML / SciMLBase.jl

The Base interface of the SciML ecosystem
https://docs.sciml.ai/SciMLBase/stable
MIT License
118 stars 91 forks source link

Remove check in `check_error!` #670

Closed visr closed 2 months ago

visr commented 2 months ago

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?

ChrisRackauckas commented 2 months ago

Does this allocation get elided?

visr commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

ChrisRackauckas commented 2 months ago

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

ChrisRackauckas commented 2 months ago

That should be sufficient.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.