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.

Sign up for GitHub

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

Jump to bottom

Should to_vec(::Integer) return an empty vector #188

Open
mzgubic opened this issue Aug 4, 2021 · 8 comments · May be fixed by #189
Open

Should to_vec(::Integer) return an empty vector #188

mzgubic opened this issue Aug 4, 2021 · 8 comments · May be fixed by #189

Comments

@mzgubic
Copy link
Member

mzgubic commented Aug 4, 2021

Integers can't be perturbed, so their to_vec should be empty. Originally suggested in #187

function to_vec(x::Integer)
     Integer_from_vec(v) = x
     return Bool[], Integer_from_vec
end

Would also fix (a part of) chrisbrahms/Hankel.jl#27

Did not test yet whether it messes up any existing ChainRules tests.
This was referenced Aug 4, 2021
Merged
Merged
@willtebbutt
Copy link
Member

This would make sense to me.

@mzgubic mzgubic linked a pull request Aug 4, 2021 that will close this issue
Open
@oxinabox
Copy link
Member

oxinabox commented Oct 1, 2021 •
edited
Loading

The practical problem with this, is that sometimes people want to test special case methods that take integers.
Which is just all round hard, since when finite diferencing they don't get integers.

But they might still want to test it against custom rules that do.
So I wonder if we really need to handle this better in CRTU or somewhere in FiniteDifferences.

Right now we often (always?) error for it

@oxinabox oxinabox mentioned this issue Oct 1, 2021
Open
@willtebbutt
Copy link
Member

Could you provide an example @oxinabox ? I'm not really clear what sort of thing you're imagining.

@oxinabox
Copy link
Member

oxinabox commented Oct 1, 2021

For example

function rrule(::typeof(sinpi), x)
    sinpi_pullback(dy) = -pi*cospi(x)
    foo(x), sinpi_pullback
end

function rrule(::typeof(sinpi), x::Integer)
    sinpi_pullback(dy) = Zero()
    foo(x), sinpi_pullback
end

@willtebbutt
Copy link
Member

And what we're proposing to do here would solve this problem, yes?

@oxinabox
Copy link
Member

oxinabox commented Oct 1, 2021

Would it? Cool!

@mcabbott
Copy link
Member

mcabbott commented Oct 1, 2021

Isn't this example the thing which already works? Integer taken to mean non-differentiable:

julia> mysinpi(x) = sinpi(x)
mysinpi (generic function with 1 method)

julia> function ChainRulesCore.rrule(::typeof(mysinpi), x)
           sinpi_pullback(dy) = NoTangent(), pi*cospi(x)*dy
           sinpi(x), sinpi_pullback
       end

julia> function ChainRulesCore.rrule(::typeof(mysinpi), x::Integer)
           sinpi_pullback(dy) = NoTangent(), NoTangent()
           sinpi(x), sinpi_pullback
       end

julia> test_rrule(mysinpi, 1.23)
Test Summary:                  | Pass  Total
test_rrule: mysinpi on Float64 |    7      7
Test.DefaultTestSet("test_rrule: mysinpi on Float64", Any[], 7, false, false)

julia> test_rrule(mysinpi, 1)
Test Summary:                | Pass  Total
test_rrule: mysinpi on Int64 |    6      6
Test.DefaultTestSet("test_rrule: mysinpi on Int64", Any[], 6, false, false)

@willtebbutt
Copy link
Member

Oh, yeah, this particular example is. I think stuff would need to be nested in order to break it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

make to_vec(::Integer) an empty vector JuliaDiff/FiniteDifferences.jl
4 participants
@willtebbutt @oxinabox @mzgubic @mcabbott