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

Implement rand_tangent and difference #91

Merged
merged 16 commits into from
Jul 11, 2020
Merged

Implement rand_tangent and difference #91

merged 16 commits into from
Jul 11, 2020

Conversation

willtebbutt
Copy link
Member

@willtebbutt willtebbutt commented Jul 5, 2020

This PR lays some foundations for (hopefully) moving FiniteDifferences over to ChainRules types, and contains exactly two things:

  1. Moves rand_tangent over from ChainRulesTestUtils. It turns out that it's useful here, and since ChainRulesTestUtils depends on this package, there shouldn't be any harm implementing them here.
  2. Implements difference, which is more or less what you might expect. If the docstring doesn't make it clear, it would be good to know.

edit: Also makes docs build on the latest Julia version.

edit-2: Also changes versions on which this is tested to 1.0 and 1, and disallows 1.3 failure on nightly. I'm guessing this just hadn't been updated for some reason.

@willtebbutt willtebbutt added the enhancement New feature or request label Jul 5, 2020

@testset "difference" begin

@testset "$(typeof(x))" for (ε, x) in [
Copy link
Member Author

Choose a reason for hiding this comment

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

These tests cover everything covered in the to_vec tests other than Dict, which doesn't currently play nicely / I'm not entirely sure how best to construct a tangent, nor how to add it to the primal.

@willtebbutt
Copy link
Member Author

willtebbutt commented Jul 6, 2020

CI failure is weird, pretty sure it's a one-off.

edit: by which I mean it's ready for review

src/difference.jl Show resolved Hide resolved
test/rand_tangent.jl Show resolved Hide resolved
test/difference.jl Show resolved Hide resolved
test/rand_tangent.jl Outdated Show resolved Hide resolved
@willtebbutt
Copy link
Member Author

Looks like the Manifest probably just needed re-generating -- it uses a relative path to get the correct version of FiniteDifferences. Does anyone know if this is the right way to do things (still?)?

@oxinabox
Copy link
Member

oxinabox commented Jul 7, 2020

It is afaik.

@oxinabox oxinabox requested review from nickrobinson251 and removed request for oxinabox July 10, 2020 11:46
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.

one small suggestion. otherwise LGTM - thanks!

test/difference.jl Show resolved Hide resolved
@willtebbutt
Copy link
Member Author

@nickrobinson251 this PR no longer exports difference or rand_tangent. Are you on board with this? I'm keen not to document / export this functionality for now because we don't strictly need it until #97 is closer to completion. It's possible that we'll realise something here isn't quite right, and I would prefer not to force us to make multiple minor releases.

@willtebbutt willtebbutt merged commit 53bd1d1 into master Jul 11, 2020
@willtebbutt willtebbutt deleted the wct/cr-types branch July 11, 2020 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants