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

Use ChainRulesTestUtils.jl #167

Merged
merged 1 commit into from
Feb 7, 2020
Merged

Use ChainRulesTestUtils.jl #167

merged 1 commit into from
Feb 7, 2020

Conversation

mattBrzezinski
Copy link
Member

@mattBrzezinski mattBrzezinski commented Feb 3, 2020

ChainRulesTestUtils.jl is now in the general registry, this is working towards using that package as a test dependency, see #166

@oxinabox
Copy link
Member

oxinabox commented Feb 3, 2020

I suspect this is because ChainRules only allows FiniteDifferences v0.7
but ChainRulesTestUtils allows FiniteDifferences v0.7, 0.8, and 0.9.

We should add tests to ChainRulesTestUtils to prove it supports everything.

In the short term maybe a patch release that cuts support down to v0.7 in ChainRulesTestUtils and a then lower bounding to that patch.

@mattBrzezinski
Copy link
Member Author

mattBrzezinski commented Feb 3, 2020

I suspect this is because ChainRules only allows FiniteDifferences v0.7
but ChainRulesTestUtils allows FiniteDifferences v0.7, 0.8, and 0.9.

We should add tests to ChainRulesTestUtils to prove it supports everything.

In the short term maybe a patch release that cuts support down to v0.7 in ChainRulesTestUtils and a then lower bounding to that patch.

When running this branch and doing add FiniteDifferences@0.7 the tests still fail. Looking into the Manifest.toml I can see that the FiniteDifferences 0.7.2 is set, but ChainRulesTestUtils does not appear.

This is probably because it's a test only dependency. But I wonder how Julia handles this under the hood, when does it use the highest version of FiniteDifferences for ChainRulesTestUtils? Or the one already in the Manifest.toml?

EDIT: Just discussed with EricD about this, everything should be using FiniteDifferences@0.7 since it's being set in ChainRules.

@oxinabox
Copy link
Member

oxinabox commented Feb 3, 2020

Hmm, this will require further investigation then.

I was fairly sure we were careful not to change anything in the source, right?

@mattBrzezinski
Copy link
Member Author

@willtebbutt

@nickrobinson251
Copy link
Contributor

The stacktrace leads to the rrule_test() and more specifically this line:

x̄s_fd = j′vp(fdm, f, ȳ, xs...)

Shouldn't that j'vp be _make_fdm_call?

@mattBrzezinski
Copy link
Member Author

Using _make_fdm_call fails the existing test in ChainRulesTestUtils.jl with

  MethodError: no method matching _make_fdm_call(::FiniteDifferences.var"#30#31"{FiniteDifferences.var"#42#43"{FiniteDifferences.var"#55#56"{FiniteDifferences.var"#57#58"{var"#fst#5"},FiniteDifferences.var"#77#79"{Tuple{Float64,Float64},Tuple{Array{Float64,1},Array{Float64,1}},Tuple{typeof(first),typeof(first)},Array{Int64,1}}},Int64},Array{Float64,1},Array{Float64,1},Int64}, ::Float64)
  Closest candidates are:
    _make_fdm_call(::Any, ::Any, !Matched::Any, !Matched::Any, !Matched::Any) at /Users/mattbr/.julia/dev/ChainRulesTestUtils.jl/src/ChainRulesTestUtils.jl:18

@mattBrzezinski mattBrzezinski marked this pull request as ready for review February 6, 2020 14:43
@mattBrzezinski mattBrzezinski changed the title Use ChainRulesTestUtils.jl WIP: Use ChainRulesTestUtils.jl Feb 6, 2020
@mattBrzezinski mattBrzezinski changed the title WIP: Use ChainRulesTestUtils.jl Use ChainRulesTestUtils.jl Feb 6, 2020
Project.toml Outdated Show resolved Hide resolved
@oxinabox
Copy link
Member

oxinabox commented Feb 6, 2020

Other than this last thing it LGTM

@mattBrzezinski
Copy link
Member Author

@nickrobinson251 any comments?

Copy link
Contributor

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

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

🚀

@@ -1,6 +1,8 @@
using Base.Broadcast: broadcastable
using ChainRules
using ChainRulesCore
using ChainRulesTestUtils
using ChainRulesTestUtils: _fdm
Copy link
Contributor

Choose a reason for hiding this comment

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

we should open an issue on ChainRulesTestUtils about whether or not we want this to be exported / part of the public API

@mattBrzezinski mattBrzezinski merged commit 395ba0c into master Feb 7, 2020
@oxinabox oxinabox deleted the MB/test-utils branch August 17, 2020 07:49
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.

None yet

3 participants