-
Notifications
You must be signed in to change notification settings - Fork 93
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #339 +/- ##
=======================================
Coverage 97.64% 97.64%
=======================================
Files 18 18
Lines 1018 1018
=======================================
Hits 994 994
Misses 24 24
Continue to review full report at Codecov.
|
There was a problem hiding this 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 |
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this 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 |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...) |
There was a problem hiding this comment.
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.
Co-authored-by: Miha Zgubic <mzgubic@users.noreply.github.com>
…ger...) Co-authored-by: Miha Zgubic <mzgubic@users.noreply.github.com>
Thanks @mzgubic ! |
Addresses #253, following @oxinabox.
I left out the last three rand definitions in light of #263 (?)