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

More restrictive @non_differentiable rand #263

Merged
merged 16 commits into from
Sep 9, 2020
Merged

More restrictive @non_differentiable rand #263

merged 16 commits into from
Sep 9, 2020

Conversation

nmheim
Copy link
Contributor

@nmheim nmheim commented Sep 8, 2020

Addresses #262. I am really not sure if this is a good approach because there are quite a few different rand methods...
This is what works with the use cases I have. What are the most common uses of rand that should be non-differentiable?

@willtebbutt willtebbutt requested a review from oxinabox September 8, 2020 15:25
@oxinabox
Copy link
Member

oxinabox commented Sep 8, 2020

Why not just add:

@non_differentiable rand()
@non_differentiable rand(::Any, ::Any, ::Any, ::Any, ::Any, ::Any)

?

rand(::Any...) is in the list of things to add when we have varargs support.
#253

It could also be added with varargs directly, without using the macro.

@willtebbutt
Copy link
Member

@oxinabox see #262?

@oxinabox
Copy link
Member

oxinabox commented Sep 8, 2020

Oh i was confused by the title.

@oxinabox oxinabox changed the title Less restrictive @non_differentiable rand More restrictive @non_differentiable rand Sep 8, 2020
@non_differentiable rand(::Int, ::Int)
@non_differentiable rand(::Int, ::Int, ::Int)
@non_differentiable rand(::Int, ::Int, ::Int, ::Int)
@non_differentiable rand(::Int, ::Int, ::Int, ::Int, ::Int)
Copy link
Member

Choose a reason for hiding this comment

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

I think all the Int should be Integer
to match:

[69] rand(X, d::Integer, dims::Integer...) in Random at /usr/local/src/julia/julia-1.5/usr/share/julia/stdlib/v1.5/Random/src/Random.jl:283

@oxinabox
Copy link
Member

oxinabox commented Sep 8, 2020

Can we add a test that makes sure
rrule and frule for something that we don't want to have mark as @non_differentiable returns nothing (i.e. no rule)

@oxinabox
Copy link
Member

oxinabox commented Sep 8, 2020

I think we might also in particular want:

@non_differentiable rand(::AbstractRNG, ::Random.Sampler)
@non_differentiable rand(::AbstractRNG, ::Tuple)

@nmheim
Copy link
Contributor Author

nmheim commented Sep 8, 2020

Let me know what you think about my take on testing the new rules. I am really not sure if this is how it should be done. Especially because the rrules and frules return Zero or DoesNotExist...

@nmheim nmheim marked this pull request as ready for review September 8, 2020 21:51
Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

Nice.
A few suggestions,
once those are addressed and the version number bumped we can merge this and tag a release

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

Just a couple of style things.

Once @oxinabox 's suggestions and these are attended to, please bump the patch version and we'll merge when tests pass :)

Thanks for the contribution -- it's much appreciated!

test/rulesets/Random/random.jl Outdated Show resolved Hide resolved
test/rulesets/Random/random.jl Outdated Show resolved Hide resolved
@nmheim
Copy link
Contributor Author

nmheim commented Sep 9, 2020

Great, thanks for the suggestions! I think the tests are failing because of something unrelated to this PR though... they also fail in #261 because of something getindex related...

@willtebbutt
Copy link
Member

Great, thanks for the suggestions! I think the tests are failing because of something unrelated to this PR though... they also fail in #261 because of something getindex related...

Oh, yeah, don't worry about the Zygote integration tests -- Zygote master is currently broken.

@nmheim
Copy link
Contributor Author

nmheim commented Sep 9, 2020

Actually, I am not sure about the comment on the test of the "differentiable" cases.
I used rand(ones(2,2)) as a test because we don't need an additional dependency like this, but I don't know why rand of an array works at all...
A better test case would be to use e.g. a distribution. Should I add that?

@oxinabox
Copy link
Member

oxinabox commented Sep 9, 2020

A better test case would be to use e.g. a distribution. Should I add that?

Yes.
Or maybe a fake distribution by defining our own struct,
and putting it in the same position a distribution argument would go.
(that way we avoid the test dependency)

Copy link
Contributor Author

@nmheim nmheim left a comment

Choose a reason for hiding this comment

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

I am sorry, but I am a bit clueless how to test the rules on fields of a struct if they don't appear in the rand function call... this is probably not how it should be done...

Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

I am going to add my suggestions and then if i didn't break CI merge this

test/rulesets/Random/random.jl Outdated Show resolved Hide resolved
test/rulesets/Random/random.jl Outdated Show resolved Hide resolved
test/rulesets/Random/random.jl Outdated Show resolved Hide resolved
test/rulesets/Random/random.jl Outdated Show resolved Hide resolved
@nmheim
Copy link
Contributor Author

nmheim commented Sep 9, 2020

Awesome, thank you! :)

@oxinabox oxinabox merged commit c8679c6 into JuliaDiff:master Sep 9, 2020
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