Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make work on functors #170

Merged
merged 20 commits into from
Jun 9, 2021
Merged

Make work on functors #170

merged 20 commits into from
Jun 9, 2021

Conversation

mzgubic
Copy link
Member

@mzgubic mzgubic commented Jun 8, 2021

Closes #117

To do:

  • merge and tag FiniteDifferences PR, add compat. Needed to generate rand_tangent for f when testing constructors, i.e. rand_tangent(Foo)
  • implement test_frule for functors
  • implement test_scalar for functors
  • remove _ensure_not_running_on_functor

src/testers.jl Outdated
if accumulated_x̄ isa Union{Nothing,NoTangent} # then we marked this argument as not differentiable # TODO remove once #113
@assert x̄_fd === nothing # this is how `_make_j′vp_call` works
x̄_ad isa ZeroTangent && error(
fd_cotangents = _make_j′vp_call(fdm, (f, xs...) -> f(xs...; fkwargs...), ȳ, (f, xs...), is_ignored)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the main change, i.e. from
(xs...) -> f(xs...; fkwargs...) to
(f, xs...) -> f(xs...; fkwargs...)
making the f an argument. The rest of the changes are adding an extra cotangent for f via rand_tangent, and renaming the variables from x̄_something to something_cotangent since it also includes

@mzgubic mzgubic changed the title WIP: Make work on functors Make work on functors Jun 8, 2021
@mzgubic
Copy link
Member Author

mzgubic commented Jun 8, 2021

No clue why the Documentation test fails, and I can't reproduce it locally even though the Julia version, Documenter version, and ChainRulesCore/TestUtils versions are all the same.

@oxinabox
Copy link
Member

oxinabox commented Jun 8, 2021

No clue why the Documentation test fails, and I can't reproduce it locally even though the Julia version, Documenter version, and ChainRulesCore/TestUtils versions are all the same.

Hack to solve this.
Lets install the GHA that makes suggestions to fix doc-tests.
We can copy it from: beacon-biosignals/KeywordSearch.jl#18

We kinda want to do that anyway.

If you need to get this merged can change that bit of the docs to be a

```julia

block rather than a

```@doctest

block.

Else can make another PR to do that stuff, and then rebase this on top of it.

Your call

@mzgubic
Copy link
Member Author

mzgubic commented Jun 9, 2021

No clue why the Documentation test fails, and I can't reproduce it locally even though the Julia version, Documenter version, and ChainRulesCore/TestUtils versions are all the same.

Hack to solve this.
Lets install the GHA that makes suggestions to fix doc-tests.
We can copy it from: beacon-biosignals/KeywordSearch.jl#18

We kinda want to do that anyway.

If you need to get this merged can change that bit of the docs to be a

```julia

block rather than a

```@doctest

block.

Else can make another PR to do that stuff, and then rebase this on top of it.

Your call

The problem is that I can't find the place where I should make the change.😬 The doctest that is meant to be failing does not exist explicitly in the code anywhere (not in CRTU, or CRC, or CR). It looks like it was generated from @autodocs but the current docs don't show it either. It's a ghost.

The failure of the doctest is that the stacktrace is coming from a different line. So it looks like it is picking up some doctest that is itself using a different version of ChainRulesTestUtils (probably master?). I wonder whether we can just try merging an see if it works?

@simeonschaub
Copy link
Member

Oh, sorry, looks like that's the doctest I introduced in #165 that's failing. I wouldn't recommend merging this before manually merging master into this branch. The failure is most likely real, because I couldn't figure out how to not make the error output I wanted to show not print the location of the testset in test_rrule. You could of course just correct the line number, but I agree that that's not a great solution, so I'm fine with not making that a doctest too.

@nickrobinson251
Copy link
Contributor

Alternatively could you Documenters Filter functionality to ignore the line number info

@mzgubic
Copy link
Member Author

mzgubic commented Jun 9, 2021

Oh, sorry, looks like that's the doctest I introduced in #165 that's failing. I wouldn't recommend merging this before manually merging master into this branch. The failure is most likely real, because I couldn't figure out how to not make the error output I wanted to show not print the location of the testset in test_rrule. You could of course just correct the line number, but I agree that that's not a great solution, so I'm fine with not making that a doctest too.

Oh, thanks! This was driving me crazy. I am still confused though - I thought CI runs on my branch? I haven't merged your changes. So it appears at least the Documenter action is running on what master would look like if this would have been merged? How does it deal with conflicts? 😬 How d...wh...? !??!? 😂

@mzgubic mzgubic merged commit 0c7884d into master Jun 9, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make work on Functors
4 participants