Skip to content

Avoid conj override message #534

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

Closed
wants to merge 5 commits into from
Closed

Conversation

dlfivefifty
Copy link
Contributor

At the moment we have

julia> using ForwardDiff
[ Info: Precompiling ForwardDiff [f6369f11-7733-5829-9624-2563aa707210]
WARNING: Method definition conj(ForwardDiff.Dual{T, V, N} where N where V) where {T} in module ForwardDiff at /Users/sheehanolver/Projects/ForwardDiff.jl/src/dual.jl:390 overwritten at /Users/sheehanolver/Projects/ForwardDiff.jl/src/dual.jl:201.
  ** incremental compilation may be fatally broken for this module **

due to conj being added to DiffRules.jl

@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2021

Codecov Report

Merging #534 (dc7cf96) into master (6b393f4) will decrease coverage by 0.12%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #534      +/-   ##
==========================================
- Coverage   84.83%   84.71%   -0.13%     
==========================================
  Files           9        9              
  Lines         831      831              
==========================================
- Hits          705      704       -1     
- Misses        126      127       +1     
Impacted Files Coverage Δ
src/dual.jl 72.37% <0.00%> (-0.35%) ⬇️

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...dc7cf96. Read the comment docs.

@@ -387,8 +387,6 @@ Base.AbstractFloat(d::Dual{T,V,N}) where {T,V,N} = convert(Dual{T,promote_type(V
# General Mathematical Operations #
###################################

@inline Base.conj(d::Dual) = d
Copy link
Collaborator

@KristofferC KristofferC Jul 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't want to drop all the previous DiffRules compat, a hasmethod check could be used instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we care about maintaining compatibility with old versions of DiffRules? My personal preference is to always drop old versions as it means less maintenance headaches.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is so easy, like in this case, might as well, or?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine, can you add the change as a suggestion? (I'm not 100% sure how to do it in a way that's precompile-compatible.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should just be

if !hasmethod(conj, Tuple{ForwardDiff.Dual})
    @inline Base.conj(d::Dual) = d
end

Not sure how to add that as a suggestion.

Copy link
Member

@mcabbott mcabbott Jul 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is hasmethod going to work here? The method you are checking for is defined in just below this in the file.

I would have guessed you should check DiffRules.diffrules() directly, which isn't changed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And, even with this mechanism, I think it needs new bounds in Project.toml before tagging.

In the registry all past versions are marked incompatible with DiffRules 1.1 & 1.2, see JuliaRegistries/General#40252 and friends. The next tagged version should probably accept <= 1.02 + >= 1.21.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you are suggestion. Perhaps make a ```suggestion ?

Copy link
Member

@mcabbott mcabbott Jul 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a PR, weeks ago. But read my comments carefully, there are two concerns. One is whether this hasmethod actually works as claimed, did you test it with DiffRules 0.9 say? The other is that tagging this would re-declare compatibility of this package with versions of DiffRules which contain 3-ary rules, which cause it to crash. I believe that can safely be done only after #530 is merged.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with the compat restricted version then. I don't think it will be any big problem in practice.

@mcabbott
Copy link
Member

mcabbott commented Jul 15, 2021

Closes #531, #532.

The other option would be to remove that from DiffRules. DiffRules mostly has holomorphic functions, although it does have abs.

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.

4 participants