trixi-framework / Trixi.jl

Trixi.jl: Adaptive high-order numerical simulations of conservation laws in Julia
https://trixi-framework.github.io/Trixi.jl
MIT License
527 stars 105 forks source link

NaN check callback #1854

Closed DanielDoehring closed 7 months ago

DanielDoehring commented 7 months ago

Having merged https://github.com/trixi-framework/Trixi.jl/pull/1781 we have return now by default NaN if there are negative arguments to sqrt and log . Opposed to the integrators from OrdinaryDiffEq.jl which come with an unstable_check that is effectively a NaN-check using the built-in any function the custom integrators in Trixi do not check by default for NaN and thus a simulation continues although it is already corrupted.

This PR adds a NaN callback which is recommended to use when using Trixi.solve.

I am not sure if we want to use @trixi_timeit timer() as the cb should be really cheap assuming an effective divide & conquer implementation of any.

github-actions[bot] commented 7 months ago

Review checklist

This checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging.

Purpose and scope

Code quality

Documentation

Testing

Performance

Verification

Created with :heart: by the Trixi.jl community.

codecov[bot] commented 7 months ago

Codecov Report

Attention: Patch coverage is 78.57143% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 89.38%. Comparing base (27d91b3) to head (a7cb1ea). Report is 1 commits behind head on main.

Files Patch % Lines
src/callbacks_step/nan.jl 76.92% 6 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1854 +/- ## ========================================== - Coverage 96.28% 89.38% -6.90% ========================================== Files 438 439 +1 Lines 35386 35401 +15 ========================================== - Hits 34071 31643 -2428 - Misses 1315 3758 +2443 ``` | [Flag](https://app.codecov.io/gh/trixi-framework/Trixi.jl/pull/1854/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trixi-framework) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/trixi-framework/Trixi.jl/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trixi-framework) | `89.38% <78.57%> (-6.90%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trixi-framework#carryforward-flags-in-the-pull-request-comment) to find out more.

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

ranocha commented 7 months ago

Shall we really use another callback for this? I think it may be better to use the same approach as OrdinaryDiffEq.jl and allow more arguments to solve - in particular an unstable_check that could just be our ode_unstable_check. See

https://github.com/trixi-framework/Trixi.jl/blob/4ee60e7973ff78d553dad65f7f4760443891bdb6/src/auxiliary/auxiliary.jl#L152-L167

and

https://github.com/trixi-framework/Trixi.jl/blob/4ee60e7973ff78d553dad65f7f4760443891bdb6/src/auxiliary/mpi.jl#L115-L130

DanielDoehring commented 7 months ago

Shall we really use another callback for this? I think it may be better to use the same approach as OrdinaryDiffEq.jl and allow more arguments to solve - in particular an unstable_check that could just be our ode_unstable_check. See

https://github.com/trixi-framework/Trixi.jl/blob/4ee60e7973ff78d553dad65f7f4760443891bdb6/src/auxiliary/auxiliary.jl#L152-L167

and

https://github.com/trixi-framework/Trixi.jl/blob/4ee60e7973ff78d553dad65f7f4760443891bdb6/src/auxiliary/mpi.jl#L115-L130

I briefly looked into this, but I personally find this approach less intuitive than using a callback. Plus, I think overall more code changes would be necessary.