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

Remove bad non-differentiable #604

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

willtebbutt
Copy link
Member

Resolves #603

@willtebbutt
Copy link
Member Author

Looks like this currently causes some downstream issues -- will attempt to isolate the particular rules that we need to keep Zygote and Diffractor tests passing.

@mcabbott
Copy link
Member

Diffractor never passes.

Zygote failures are things like this:

/home/runner/work/ChainRules.jl/ChainRules.jl/downstream/test/gradcheck.jl:1730
[313](https://github.com/JuliaDiff/ChainRules.jl/runs/5733081278?check_suite_focus=true#step:6:313)
  Test threw exception
[314](https://github.com/JuliaDiff/ChainRules.jl/runs/5733081278?check_suite_focus=true#step:6:314)
  Expression: gradient((x->begin
[315](https://github.com/JuliaDiff/ChainRules.jl/runs/5733081278?check_suite_focus=true#step:6:315)
                sum(rand(Random.GLOBAL_RNG, Float32, 1, 1))
[316](https://github.com/JuliaDiff/ChainRules.jl/runs/5733081278?check_suite_focus=true#step:6:316)
            end), 1) == (nothing,)

for which the immediate fix is that there should be rules something like

@non_differentiable rand(::Type{<:Number}, ::AbstractRNG, ::Tuple)
@non_differentiable rand(::Type{<:Number}, ::AbstractRNG, ::Integer...)

@devmotion
Copy link
Member

devmotion commented Mar 29, 2022

This seems suprising since the PR does not touch rand, I would assume removing rules for rand! should not affect something like sum(rand(args...))?

Edit: Ah, probably rand(Float32, 1, 1) calls rand! internally...

@mcabbott
Copy link
Member

mcabbott commented Mar 29, 2022

Yes, the method is:

rand(r::AbstractRNG, ::Type{X}, dims::Dims) where {X} = rand!(r, Array{X}(undef, dims), X)

If we allow rand!(rng, array) then it's possible we should allow rand!(rng, array, eltype) too.

I'd rather not have any of these mutating functions, but removing them might break too many things.

@marius311
Copy link

Stumbled on this after a long dive (w/ help from Zack Li and Brian Chen on Slack) trying to understand this incorrectly dropped gradient:

gradient-> sum(rand(Xoshiro(1), MvNormal(zeros(2), σ*I))), 1) # nothing

Conversely, with this PR, instead of dropping it, you (correctly) get an error since the rand code in Distributions.jl is mutating. If you load DistributionsAD, then it works. But yea would be good not to silently and incorrectly drop it, so +1 from me on this PR (at least w.r.t. to this, no comment on anything else breaking)

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.

rand! is marked non-differentiable
5 participants