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

Fix seg fault from ndf15 evolver in #419 #423

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fruzsinaagocs
Copy link

This PR fixes a possible segmentation fault arising from NaN values being passed to the numjac function of the ndf15 numerical solver when asking for the relevant derivatives (i.e. the right-hand-side of the differential equation being solved). I perform a class_test whenever the relevant function calculating the derivatives, *derivs, is called, so CLASS throws a more informative error, and doesn't break when called in a loop as part of an MCMC chain.

This fixes #419, where a shooting error is delayed until the background module, but is never actually thrown because the segmentation error occurs first. It also fixes an issue encountered in GAMBIT (https://github.com/GambitBSM), where an unphysical point in the MCMC parameter space causes one or more of the differential equations involved to be ill-defined.

Copy link
Contributor

@schoeneberg schoeneberg left a comment

Choose a reason for hiding this comment

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

Dear Fruszina,

I think this in general is a good idea, except that when we do the optimization strategy in the Makefile, usually such a check of "x!=x" would be removed by the option "--fast-math" that we had in our Makefile for a long time. As such, I would like to also request the opinion of
@ThomasTram about this, and what he thinks of such a NaN check, especially given that he wrote the ndf15.c evolver.

EDIT: I think we would also need to re-implement this as an issue in the current development branch of CLASS to make sure it gets all the proper tests etc.

Cheers!

@fruzsinaagocs
Copy link
Author

Hi Nils,

Thanks for the quick response! That's completely true, unfortunately the --fast-math option generally makes it hard to catch NaNs with a generic origin... Let's see what Thomas suggests.

@pstoecker
Copy link

Thanks for the heads-up on this Nils. This could really lead to problems.

How about using isnan() from math.h or is this also effectively optimised away with --fast-math?

@fruzsinaagocs
Copy link
Author

Unfortunately isnan() is optimised away as well. Basically, --fast-math assumes NaNs and infs never occur.

@pstoecker
Copy link

OK I see. if --ffast-math is really such a beasty flag one should really refrain from using it, unless one is 100% sure that the code will never lead to infs or NaNs (which is a very ambitious promise in a code a big as CLASS).

I guess that this is actually the reason why this particular flag is never included when we compile C/C++ codes in gambit with gcc. Seeing as --ffast-math is a shortcut for fno-math-errno -funsafe-math-optimizations -ffinite-math-only -fno-rounding-math -fno-signaling-nans -fcx-limited-range one could instead use this list and strip -ffinite-math-only (and other potentially poisonous flags) from it.

The question, however, is whether we will have to give up speed in doing so.

@ThomasTram
Copy link
Collaborator

Hi @fruzsinaagocs

Thanks for looking into this! I don't like --fast-math either, we should test how much of a speed difference it really makes. (Also perhaps for a subset of them, lige suggested by @pstoecker ) This seems like something that could be done with a little bit of bash-scripting. However, perhaps the fix could be made more robust: there must be some index calculation that fails with NaNs, but that can be fixed in such a way that it works regardless of --fast-math. Do you happen to have an .ini file that generates a segmentation-fault?

Cheers,
Thomas

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.

parameter set causing "Segmentation fault" in python wrapper
4 participants