Skip to content

fix float(::Dual) and add float(::Type{<:Dual}) #535

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

Merged
merged 1 commit into from
Jul 28, 2021

Conversation

stevengj
Copy link
Contributor

Fixes #492, fixes #362, closes #419.

The Base.float function in base supports numeric types, not just values:

julia> float(Complex{Int})
ComplexF64 (alias for Complex{Float64})

and by exploiting this we obtain floating-point promotion consistent with Base.

@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2021

Codecov Report

Merging #535 (93a85ef) into master (6b393f4) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #535      +/-   ##
==========================================
+ Coverage   84.83%   84.93%   +0.10%     
==========================================
  Files           9        9              
  Lines         831      830       -1     
==========================================
  Hits          705      705              
+ Misses        126      125       -1     
Impacted Files Coverage Δ
src/dual.jl 73.07% <100.00%> (+0.34%) ⬆️
src/apiutils.jl 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b393f4...93a85ef. Read the comment docs.

@stevengj
Copy link
Contributor Author

stevengj commented Jul 16, 2021

The

Test threw exception
  Expression: dual_isapprox(NESTED_FDNUM ^ PRIMAL, exp(PRIMAL * log(NESTED_FDNUM)))
  DomainError with -1.1858610145546527e18:
  sqrt will only return a complex result if called with a complex argument. Try sqrt(Complex(x)).

makes no sense to me. I actually got a similar error at some point locally, and then it mysteriously disappeared when I re-ran the test — makes me think it is a bug in Julia itself.

Have you seen similar errors prior to this PR?

@KristofferC
Copy link
Collaborator

I think I recall seeing something similar to that. It clearly isn't related to the PR.

@mcabbott
Copy link
Member

mcabbott commented Jul 17, 2021

Re NESTED_FDNUM ^ PRIMAL, the rule for ^ attempts to avoid a DomainError from log, and was changed recently. (Both 1.0 and 1 CI are DiffRules v1.2.1, the latest.) Is it possible this is somehow to blame? I don't see how though. https://github.com/JuliaDiff/DiffRules.jl/blob/master/src/rules.jl#L88

Reading the stacktrace more carefully, the error is within isapprox, which calls norm, so it's unrelated to the function actually being tested, which must have run by then.

Note also that I think #530 needs to be merged before tagging anything.

@stevengj
Copy link
Contributor Author

Is this mergeable, then? If not, what needs to be done?

@dlfivefifty
Copy link
Contributor

We also need Base.big. Can you add that here, or shall I do it in a separate PR?

@KristofferC KristofferC merged commit 102ee4d into JuliaDiff:master Jul 28, 2021
@stevengj stevengj deleted the betterfloat branch July 28, 2021 22:37
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.

float(::Dual) disagrees with Base ForwardDiff.derivative(float, 1) returns Float16(1.0)
5 participants