Skip to content

add varargs nondiff rules #339

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

Merged
merged 7 commits into from
Jan 6, 2021
Merged

Conversation

niklasschmitz
Copy link
Contributor

Addresses #253, following @oxinabox.

I left out the last three rand definitions in light of #263 (?)

@non_differentiable rand(::Any...)
@non_differentiable randn(::Any...)
@non_differentiable randexp(::Any...)

@codecov-io
Copy link

codecov-io commented Jan 6, 2021

Codecov Report

Merging #339 (b74e3f7) into master (e11af5d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #339   +/-   ##
=======================================
  Coverage   97.64%   97.64%           
=======================================
  Files          18       18           
  Lines        1018     1018           
=======================================
  Hits          994      994           
  Misses         24       24           
Impacted Files Coverage Δ
src/rulesets/Random/random.jl 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e11af5d...b74e3f7. Read the comment docs.

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.

Hey, thanks for taking this on, we appreciate it! The changes look great, thanks for going through the all rules.

With regard to #263: The issue was that the rand function arguments were not constrained at all (i.e. were Any), meaning that the @non_differentiable macro would apply to all the methods, even those that were differentiable in some arguments. It is still possible to reduce the number of lines by replacing, for example:

@non_differentiable rand(::AbstractRNG, ::Integer)
@non_differentiable rand(::AbstractRNG, ::Integer, ::Integer)
@non_differentiable rand(::AbstractRNG, ::Integer, ::Integer, ::Integer)
@non_differentiable rand(::AbstractRNG, ::Integer, ::Integer, ::Integer, ::Integer)
@non_differentiable rand(::AbstractRNG, ::Integer, ::Integer, ::Integer, ::Integer, ::Integer)

with

@non_differentiable rand(::AbstractRNG, ::Integer...)

If you can bump the version number as well this PR is ready to merge.

@non_differentiable rand(::Integer, ::Integer, ::Integer, ::Integer, ::Integer)

@non_differentiable rand(::Type{<:Real}, ::Integer...)
@non_differentiable rand(::Integer...)

# There are many different 1-3 arg methods, but not varargs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure about what this comment means, so I didn't replace it by rand!(::Any...)

Copy link
Member

Choose a reason for hiding this comment

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

The comment means that the function rand! has many different method definitions, which you can see by typing methods(rand!) in the REPL. However, if you look closely, none of them have a Vararg (or ...) in the arguments. So, it is correct not to replace it, as it would lead to incorrect results in case someone defined a differentiable rand! method with four arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I see, thank you!

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.

Great, thanks! Just a couple of minor things left

@non_differentiable rand(::Integer, ::Integer, ::Integer, ::Integer, ::Integer)

@non_differentiable rand(::Type{<:Real}, ::Integer...)
@non_differentiable rand(::Integer...)

# There are many different 1-3 arg methods, but not varargs
Copy link
Member

Choose a reason for hiding this comment

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

The comment means that the function rand! has many different method definitions, which you can see by typing methods(rand!) in the REPL. However, if you look closely, none of them have a Vararg (or ...) in the arguments. So, it is correct not to replace it, as it would lead to incorrect results in case someone defined a differentiable rand! method with four arguments.


@non_differentiable randn!(::AbstractArray)
@non_differentiable randn!(::AbstractRNG, ::AbstractArray)


@non_differentiable randn(::AbstractRNG)
Copy link
Member

Choose a reason for hiding this comment

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

Was this line removed by accident, or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is covered already by randn(::Any...) above

@non_differentiable randexp(::Any, ::Any, ::Any)
@non_differentiable randexp(::Any, ::Any, ::Any, ::Any)
@non_differentiable randexp(::Any, ::Any, ::Any, ::Any, ::Any)
@non_differentiable randexp(::Any...)
Copy link
Member

Choose a reason for hiding this comment

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

Great, this was actually missing a randexp() before, which is now covered.

niklasschmitz and others added 2 commits January 6, 2021 13:10
Co-authored-by: Miha Zgubic <mzgubic@users.noreply.github.com>
…ger...)

Co-authored-by: Miha Zgubic <mzgubic@users.noreply.github.com>
@mzgubic mzgubic merged commit ba7bbd0 into JuliaDiff:master Jan 6, 2021
@niklasschmitz
Copy link
Contributor Author

Thanks @mzgubic !

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.

3 participants