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

Int(::Dual) and Integer(::Dual) #508

Merged
merged 5 commits into from
Jul 9, 2021
Merged

Conversation

dlfivefifty
Copy link
Contributor

This adds convert routines for integers. I need this to fix the following bug:

julia> length(UnitRange(Dual(1.0), Dual(2.0)))
ERROR: MethodError: no method matching Integer(::Dual{Nothing, Float64, 0})
Closest candidates are:
  (::Type{T})(::T) where T<:Number at boot.jl:760
  (::Type{T})(::AbstractChar) where T<:Union{AbstractChar, Number} at char.jl:50
  (::Type{T})(::Base.TwicePrecision) where T<:Number at twiceprecision.jl:243
  ...
Stacktrace:
 [1] unsafe_length(r::UnitRange{Dual{Nothing, Float64, 0}})
   @ Base ./range.jl:559
 [2] length(r::UnitRange{Dual{Nothing, Float64, 0}})
   @ Base ./range.jl:561
 [3] top-level scope
   @ REPL[4]:1

@codecov-io
Copy link

codecov-io commented Mar 3, 2021

Codecov Report

Merging #508 (f89181b) into master (62512aa) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #508      +/-   ##
==========================================
+ Coverage   88.94%   88.96%   +0.02%     
==========================================
  Files          10       10              
  Lines         841      843       +2     
==========================================
+ Hits          748      750       +2     
  Misses         93       93              
Impacted Files Coverage Δ
src/dual.jl 79.32% <100.00%> (+0.14%) ⬆️

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 62512aa...f89181b. Read the comment docs.

@KristofferC
Copy link
Collaborator

I think this should only work if the dual part is zero (cf the behavior of complex numbers).

@dlfivefifty
Copy link
Contributor Author

I think I agree:

julia> Integer(nextfloat(1.0))
ERROR: InexactError: Int64(1.0000000000000002)
Stacktrace:
 [1] Int64
   @ ./float.jl:723 [inlined]
 [2] Integer(x::Float64)
   @ Core ./boot.jl:786
 [3] top-level scope
   @ REPL[1]:1

But in that case I should also overload unsafe_length(::UnitRange{<:Dual}) so that

julia> UnitRange(Dual(1,1), Dual(2,0)) |> collect
2-element Vector{Dual{Nothing, Int64, 1}}:
 Dual{Nothing}(1,1)
 Dual{Nothing}(2,1)

works.

@dlfivefifty
Copy link
Contributor Author

Ha! We're in luck, because partials(step(r)) == partials(first(r)) the default code for unsafe_length(::UnitRange) just works as is..

@ranocha
Copy link
Contributor

ranocha commented Mar 22, 2021

Bump. This looks like a useful addition to me 👍

@dlfivefifty
Copy link
Contributor Author

Can this be merged? Just ran into it, again

@dlfivefifty
Copy link
Contributor Author

The test failure looks unrelated...

@KristofferC
Copy link
Collaborator

KristofferC commented Jul 1, 2021

I agree that the test failures don't look directly related to this but when I look at the CI run for earlier commits on master (https://github.com/JuliaDiff/ForwardDiff.jl/commits/master) they all seem green so it is a bit surprising why this would suddenly start happening. And it happens for all Julia versions as well.

@KristofferC
Copy link
Collaborator

KristofferC commented Jul 1, 2021

CI error also happens in #527. It seems to be something with the StaticArray tests so maybe it is a new version of StaticArrays that caused it?

The last PR on master ran with StaticArrays v1.1.0 and this one runs with StaticArrays v1.2.4,

@KristofferC
Copy link
Collaborator

I bisected and JuliaArrays/StaticArrays.jl#907 is when the ForwardDiff test failure was introduced.

@dlfivefifty
Copy link
Contributor Author

Note the values depend on the sample in x = rand(3,3). If I do Random.seed!(0) I get the slightly different result:

julia> @test DiffResults.value(result1) == DiffResults.value(result)
Test Failed at REPL[39]:1
  Expression: DiffResults.value(result1) == DiffResults.value(result)
   Evaluated: 0.0 == 1.297445353195765e-6

@dlfivefifty
Copy link
Contributor Author

dlfivefifty commented Jul 1, 2021

I'm debugging this now. FYI I believe vector_mode_dual_eval should be renamed vector_mode_dual_eval!

function vector_mode_dual_eval(f!::F, y, x, cfg::JacobianConfig) where {F}

EDIT: The StaticArrays.jl#907 Issue already has a minimum working example.

@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2021

Codecov Report

Merging #508 (74e59ac) into master (909976d) will increase coverage by 0.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #508      +/-   ##
==========================================
+ Coverage   84.62%   84.83%   +0.21%     
==========================================
  Files           9        9              
  Lines         826      831       +5     
==========================================
+ Hits          699      705       +6     
+ Misses        127      126       -1     
Impacted Files Coverage Δ
src/dual.jl 72.72% <100.00%> (+0.84%) ⬆️

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 909976d...74e59ac. Read the comment docs.

@KristofferC KristofferC merged commit 6b393f4 into JuliaDiff:master Jul 9, 2021
@dlfivefifty dlfivefifty deleted the patch-1 branch July 9, 2021 12:43
@oxinabox oxinabox mentioned this pull request Jul 26, 2021
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