Skip to content

Remove dual hack#134

Merged
ChrisRackauckas merged 1 commit into
FluxML:masterfrom
devmotion:dw/remove_dual_hack
Sep 7, 2022
Merged

Remove dual hack#134
ChrisRackauckas merged 1 commit into
FluxML:masterfrom
devmotion:dw/remove_dual_hack

Conversation

@devmotion

@devmotion devmotion commented Sep 7, 2022

Copy link
Copy Markdown
Member

This PR removes a "Hack for conversions", as it's called in the comment.

The problem is that

julia> using ForwardDiff

julia> ForwardDiff.derivative(x -> Float64(x), 5)
ERROR: MethodError: no method matching Float64(::ForwardDiff.Dual{ForwardDiff.Tag{var"#3#4", Int64}, Int64, 1})
Closest candidates are:
  (::Type{T})(::Real, ::RoundingMode) where T<:AbstractFloat at rounding.jl:200
  (::Type{T})(::T) where T<:Number at boot.jl:772
  (::Type{T})(::AbstractChar) where T<:Union{AbstractChar, Number} at char.jl:50
  ...
Stacktrace:
 [1] (::var"#3#4")(x::ForwardDiff.Dual{ForwardDiff.Tag{var"#3#4", Int64}, Int64, 1})
   @ Main ./REPL[3]:1
 [2] derivative(f::var"#3#4", x::Int64)
   @ ForwardDiff ~/.julia/packages/ForwardDiff/pDtsf/src/derivative.jl:14
 [3] top-level scope
   @ REPL[3]:1

julia> using Tracker

julia> ForwardDiff.derivative(x -> Float64(x), 5)
ERROR: Cannot determine ordering of Dual tags Nothing and ForwardDiff.Tag{var"#5#6", Int64}
Stacktrace:
 [1] (a::Type, b::Type)
   @ ForwardDiff ~/.julia/packages/ForwardDiff/pDtsf/src/dual.jl:54
 [2] partials
   @ ~/.julia/packages/ForwardDiff/pDtsf/src/dual.jl:108 [inlined]
 [3] extract_derivative
   @ ~/.julia/packages/ForwardDiff/pDtsf/src/derivative.jl:84 [inlined]
 [4] derivative(f::var"#5#6", x::Int64)
   @ ForwardDiff ~/.julia/packages/ForwardDiff/pDtsf/src/derivative.jl:14
 [5] top-level scope
   @ REPL[5]:1

@coveralls

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 3010223394

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 72.353%

Totals Coverage Status
Change from base Build 2905161731: 0.2%
Covered Lines: 492
Relevant Lines: 680

💛 - Coveralls

@codecov-commenter

codecov-commenter commented Sep 7, 2022

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.35%. Comparing base (187b472) to head (f3130be).
⚠️ Report is 68 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   72.14%   72.35%   +0.21%     
==========================================
  Files           9        9              
  Lines         682      680       -2     
==========================================
  Hits          492      492              
+ Misses        190      188       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ChrisRackauckas

Copy link
Copy Markdown
Member

Yes, the fact that this messes up the tags means it's not even useful, and is dangerous. I'm merging and tagging this. The nightly failure looks unrelated.

@ChrisRackauckas ChrisRackauckas merged commit aef6633 into FluxML:master Sep 7, 2022
@devmotion devmotion deleted the dw/remove_dual_hack branch September 7, 2022 19:43
@coveralls

coveralls commented Nov 5, 2024

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 3010223394

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 72.353%

Totals Coverage Status
Change from base Build 2905161731: 0.2%
Covered Lines: 492
Relevant Lines: 680

💛 - Coveralls

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