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

Make work on Functors #117

Closed
oxinabox opened this issue Feb 4, 2021 · 6 comments · Fixed by #170
Closed

Make work on Functors #117

oxinabox opened this issue Feb 4, 2021 · 6 comments · Fixed by #170
Milestone

Comments

@oxinabox
Copy link
Member

oxinabox commented Feb 4, 2021

right now all the tests check they are not funning on functors.

With the new API in #116
we can have f ⟂ ḟ and `f ⟂ f̄``, which would be fine API-wise.

@oxinabox oxinabox added this to the v1 milestone Feb 17, 2021
@piever
Copy link

piever commented Feb 17, 2021

Current work around (discussed on Slack) is to cheat a bit and define an intermediate function, that is

(f::F)(x) = compute(f, x)

function compute(f::F, x)
    # The actual callable struct implementation
end

function ChainRulesCore.rrule(::typeof(compute), f::F, x)
    # Compute value and pullback function
end

# Now testing works fine
test_rrule(compute, f, x)

@rick2047
Copy link

Why can't we facilitate this work around in the code itself? Work around seems to be simple enough that instead of checking for a functor we can wrap it ourselves?

@oxinabox
Copy link
Member Author

The reason this issue is still open is that it hasn't been done yet.
Because noone has gotten round to making a PR.
Feel encouraged to open one, contributions are welcome. :-)

We don't need the work around in the code itself. when we can fix it properly.
Which is mostly just making sure things are fed to FiniteDifferences.jl correctly and removing the check.
Although, I am not 100% sure that that doesn't actually look a lot like the workaround. It might, it might not.

@rick2047
Copy link

if you provide me a few pointers on what is meant by "fed to FiniteDifferences.jl correctly", I can open a PR. Maybe some documentation?

@oxinabox
Copy link
Member Author

Working out exactly what it is to fed to FiniteDIfferences.jl correctly, is kinda the hard part of the PR.

The actual methods we hit in FiniteDifferences.jl are these ones:
https://github.com/JuliaDiff/FiniteDifferences.jl/blob/c31952549df3607c4809d141c533d83d0015e23a/src/grad.jl#L49-L76
Which use to_vec to deal with types that are not vectors.
That already should support functors just fine
https://github.com/JuliaDiff/FiniteDifferences.jl/blob/c31952549df3607c4809d141c533d83d0015e23a/src/to_vec.jl#L26-L41
but the finite differences method are not prepared to accept functors as their f arg.
Which does mean something like #117 (comment)
is wanted.

Probably adapting the _wrap_function in https://github.com/JuliaDiff/ChainRulesTestUtils.jl/blob/master/src/finite_difference_calls.jl to do that is right.
That file is kinda gross as is (which means making it more gross is less of a problem).
That whole thing (and a lot of the FiniteDifferences file) will one day be cleaned up more extensively by JuliaDiff/FiniteDifferences.jl#97
but that is a much bigger project.
And having this work even if a bit ugly is useful for that to inform that future FiniteDifferences.jl rewrite.

The other thing that is need is to workout what (if anything) need to be done to the make_jvp_calls

function _make_jvp_call(fdm, f, y, xs, ẋs, ignores)

Mayube that should will change from:
_make_jvp_call(fdm, f, y, xs, ẋs, ignores)
to _make_jvp_call(fdm, y, xs, ẋs, ignores) with f just being the first element of xs and its derivative the first of ẋs and similar it having a entry in ignores.
similar for _make_j′vp_call.
and those changes would need to be propegated upwards to the places that call those.
Basically so that most of the code in this package doesn't treat f any different from any of its arguments.

@rick2047
Copy link

Ok I gave it my best but I can't figure it out. Thanks for the explanation though. Maybe good for the next person.

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 a pull request may close this issue.

3 participants