-
Notifications
You must be signed in to change notification settings - Fork 151
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
At the moment we have
due to
conj
being added to DiffRules.jl