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

NaN-Safe Branch #451

Closed
wants to merge 1 commit into from
Closed

NaN-Safe Branch #451

wants to merge 1 commit into from

Conversation

ChrisRackauckas
Copy link
Member

This is a branch with NaN-safe mode enabled to make it easier for folks to get this turned on. We can just keep rebasing as necessary. It's good for helping people make MWEs who don't necessarily know a bunch of dev stuff.

This is a branch with NaN-safe mode enabled to make it easier for folks to get this turned on. We can just keep rebasing as necessary. It's good for helping people make MWEs who don't necessarily know a bunch of dev stuff.
@asinghvi17
Copy link

Would it make sense to read an ENV variable at precompile time instead?

@KristofferC
Copy link
Collaborator

Another possibility is just to exploit the Julia #265 fix and have something like

nan_safe() = false
nan_safe(v::Bool) = @eval nan_safe() = $v

@inline function Base.:*(partials::Partials, x::Real)
    x = nansafe() ? x : ifelse(!isfinite(x) && iszero(partials), one(x), x)
    return Partials(scale_tuple(partials.values, x))
end

and you can just toggle nan_safe e.g. in the REPL.

@andreasnoack
Copy link
Member

I'm in favor of making this the default while working on an easier way to opt out. I've never understood why it's okay to have a wrong default here and it's really frustrating to hit these cases.

@devmotion
Copy link
Member

Fixed by #539 and released in 0.10.21.

@devmotion devmotion closed this Oct 14, 2021
@KristofferC
Copy link
Collaborator

Maybe we should also benchmark again and see what the performance impact is now and maybe enable it by default..

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.

5 participants