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.

Sign up for GitHub

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
dlfivefifty wants to merge 5 commits into from
Closed

Avoid conj override message #534

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dd075b0
Avoid conj override message
dlfivefifty Jul 15, 2021
9c66dd2
remove conj override
dlfivefifty Jul 15, 2021
a1b7aeb
Revert "Avoid conj override message"
dlfivefifty Jul 15, 2021
b64e3f1
Revert "remove conj override"
dlfivefifty Jul 15, 2021
dc7cf96
Update dual.jl
dlfivefifty Jul 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/dual.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,9 @@ 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 •
edited
Loading

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

@dlfivefifty dlfivefifty Jul 15, 2021

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

@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 it is so easy, like in this case, might as well, or?

Copy link
Contributor Author

@dlfivefifty dlfivefifty Jul 15, 2021

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

@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.

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 •
edited
Loading

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

@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.

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

@dlfivefifty dlfivefifty Jul 17, 2021

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 •
edited
Loading

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

@KristofferC KristofferC Jul 28, 2021

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.

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

for (M, f, arity) in DiffRules.diffrules()
in((M, f), ((:Base, :^), (:NaNMath, :pow), (:Base, :/), (:Base, :+), (:Base, :-))) && continue
Expand Down