Skip to content

add varargs nondiff rules - #339

Merged
mzgubic merged 7 commits into
JuliaDiff:masterfrom
niklasschmitz:nondiff-varargs
Jan 6, 2021
Merged

add varargs nondiff rules#339
mzgubic merged 7 commits into
JuliaDiff:masterfrom
niklasschmitz:nondiff-varargs

Conversation

@niklasschmitz

Copy link
Copy Markdown
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

codecov-io commented Jan 6, 2021

Copy link
Copy Markdown

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.

@mzgubic mzgubic left a comment

Copy link
Copy Markdown
Member

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(::Type{<:Real}, ::Integer...)
@non_differentiable rand(::Integer...)

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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!

@mzgubic mzgubic left a comment

Copy link
Copy Markdown
Member

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(::Type{<:Real}, ::Integer...)
@non_differentiable rand(::Integer...)

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

Copy link
Copy Markdown
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!(::AbstractRNG, ::AbstractArray)


@non_differentiable randn(::AbstractRNG)

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread src/rulesets/Random/random.jl Outdated
Comment thread src/rulesets/Random/random.jl Outdated
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
Copy Markdown
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