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

Use eltype to determine the proper zero to fill! in _zerolike_writeat #659

Merged
merged 1 commit into from
Aug 10, 2022

Conversation

SBuercklin
Copy link
Contributor

@SBuercklin SBuercklin commented Aug 9, 2022

Fixes, e.g, the example below:

using Unitful, UnitfulChainRules
using Zygote

Zygote.gradient(A -> ustrip.(maximum(A)), [1.0, 2.0, 3.0] * u"m")
# ERROR: DimensionError: m^-1 and 0 are not dimensionally compatible.

This occurs because we hardcode the 0 in _zerolike_writeat, which ends up getting called through the maximum rrule, among others. Using zero(eltype(dy)) we can fill with the proper 0-like.

I included a check just in case eltype(dy) == Any, in which case we default to the previous behavior as zero(Any) is undefined.

Is there a better way to determine when zero(eltype(dy)) wouldn't be valid? Maybe defaulting to ZeroTangent() would be better in the case where eltype doesn't concretely define a zero?

Copy link
Member

@mzgubic mzgubic left a comment

Choose a reason for hiding this comment

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

Thanks!

Is there a better way to determine when zero(eltype(dy)) wouldn't be valid?

I agree it is not ideal at the moment, but it is an improvement over the status quo, so I am happy for it to be merged as is. Looks like zero works for Unions of Number subtypes, and I couldn't really think of any other edge cases where it would fail. Something like

_zerofill = try zero(eltype(dy)) catch e 0 end

would be better, it doesn't harm inference, but I think Zygote will fail to do a second order derivative through this.

Maybe defaulting to ZeroTangent() would be better in the case where eltype doesn't concretely define a zero?

I'd prefer the current implementation since we likely don't want something like this:

julia> ChainRules._zerolike_writeat([1, 2, 3.0], Any[1.0], 1, 3)
3-element Vector{Any}:
  ChainRulesCore.ZeroTangent()
  ChainRulesCore.ZeroTangent()
 1.0

@mzgubic mzgubic merged commit 8369959 into JuliaDiff:main Aug 10, 2022
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.

2 participants