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

Using a strong zero in Dual(0.0, 1)^0 to avoid NaN #84

Merged
merged 8 commits into from
Apr 3, 2021

Conversation

jwscook
Copy link
Contributor

@jwscook jwscook commented Mar 19, 2021

I found another minor bug-ette:

julia> Dual(0.0, 1)^0
Dual{Float64}(1.0,NaN)

The behaviour from this PR gives:

julia> Dual(0.0, 1)^0
1.0 - 0.0ɛ

What do you reckon?

@dlfivefifty
Copy link
Collaborator

Shouldn't this also work for (::Dual)^0.0 based on:

julia> 0.0^0.0
1.0

julia> h = 0.000001; ((0+h)^0.0 - 0.0^0.0)/h
0.0

Also make sure there are tests for dual(0,0)^0 (which should return dual(1,0)) as well as 0.0^dual(0,1) (which should return dual(1,NaN)) and finally dual(0,1)^dual(0,1) should return dual(1,0).

@jwscook
Copy link
Contributor Author

jwscook commented Mar 19, 2021

Pleases and thank yous go a long way. Dual(0, 0)^0 should return a DomainError - see the tests.

@mlubin
Copy link
Contributor

mlubin commented Mar 19, 2021

For reference, this is a longstanding and known issue, see https://github.com/JuliaDiff/ForwardDiff.jl/blob/3fb16ac21ac06b7c42c807e898395ad3bd9f54ae/docs/src/user/advanced.md#fixing-naninf-issues. I guess DualNumbers isn't especially focused on performance, so the 5%-10% overhead may be acceptable.

@dlfivefifty
Copy link
Collaborator

Dual(0, 0)^0 should return a DomainError

Why?

julia> 0^0
1

julia> (0+eps())^0
1.0

@jwscook
Copy link
Contributor Author

jwscook commented Mar 21, 2021

In the previous implementation Dual(0, 1)^0 performed a 0^-1 which threw a DomainError. Dual{T} where {T<:Integer} still throws rather than promote to float.

I've also fixed the type instability of Dual^Dual to promote from Dual{T} where {T<:Integer} to Dual{T} where {T<:AbstractFloat} because it has to use a log(::Integer) anyway.

@jwscook
Copy link
Contributor Author

jwscook commented Mar 31, 2021

Is there anything remaining that needs to be done before this is merged (other than semver bump)?

@dlfivefifty
Copy link
Collaborator

Pleas do the semvar bump

@dlfivefifty dlfivefifty merged commit 4603cc1 into JuliaDiff:master Apr 3, 2021
This pull request was closed.
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.

3 participants