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 test_utils available as a stand alone package #69

Closed
nickrobinson251 opened this issue Aug 2, 2019 · 21 comments
Closed

Make test_utils available as a stand alone package #69

nickrobinson251 opened this issue Aug 2, 2019 · 21 comments
Assignees
Milestone

Comments

@nickrobinson251
Copy link
Contributor

nickrobinson251 commented Aug 2, 2019

  • The test utils that use finite differences to check correctness are nice, and probably could be useful for people adding sensitivites in their own packages.
  • We could make this a submodule, like ChainRules.TestUtils so people can use this functionality in their own tests
  • These utils depend on FDM/FiniteDifferences.jl, which is probably not a dependency we want to add to ChainRulesCore.jl, but is less bad here where we already have heavier dependencies (and it'd be a test-only dependency for users)
  • If this functionality does turn out to be useful and widely used, then we can easily make this its own package in future
@oxinabox
Copy link
Member

oxinabox commented Aug 2, 2019

I'm not super against adding a FiniteDifferences.jl dependency to ChainRules.jl
because it means that we can probably do something like Zygote's validate flag,
where it checked custom sensitivies everytime they are called.

Though perhaps that is better done in the AD only

@nickrobinson251
Copy link
Contributor Author

I wonder what @willtebbutt thinks?

@willtebbutt
Copy link
Member

I guess there are two options here:

  1. Add a ChainRules.TestUtil module, and make FiniteDifferences a dependency.
  2. Create another repo called ChainRulesTestUtil or something. This might be preferable from a package maintainer's point of view as they could then just add ChainRulesCore to their [deps], and ChainRulesTestUtil to their [extras], and not depend on ChainRules at all.

because it means that we can probably do something like Zygote's validate flag

This is something that we could think about doing, but I expect that the merits are much less substantial than in an AD-package's case.

@oxinabox
Copy link
Member

oxinabox commented Aug 3, 2019

Option 3.
We could make TestUtil a submodule of FDM.
We would want to make it a bit more general so you need to pass in that you are using something to get the rules and stuff. (Possibly could be made general enough that you easily use it for testing both chainrules and an AD system). This feels good if we can get a good abstraction.
Maybe something that takes in such a thing and returns a named tuple of closures over it, as a kind of fake module?

@willtebbutt
Copy link
Member

willtebbutt commented Aug 3, 2019

I would be pro- this third option if it could be done in a way that has a simple API that would play nicely with both ChainRules and AD packages. Seems like a common-enough problem.

edit: I guess the pertinent question is how much additional code would one need to implement in ChainRules or Zygote for this, on top of whatever we add in FiniteDifferences.

@willtebbutt
Copy link
Member

Just bumping this. Is anyone keen to work on it? I would love to see it done, as I regularly implement custom adjoints and having something standalone to help out with this would be fantastic. Alas, I don't really have time to work on this at the minute.

@oxinabox
Copy link
Member

oxinabox commented Nov 4, 2019

I have limitted time to do ChainRules stuff right now,
In a few weeks will have plenty of time and sometime after that will have both time and people.

What little time I have right now, is direct at the Composite PR in ChainRulesCore, and the Zygote PR.

@oxinabox oxinabox added this to the v1 milestone Nov 29, 2019
@oxinabox oxinabox changed the title Make test_utils available as a submodule Make test_utils available as a stand alone package Jan 4, 2020
@oxinabox
Copy link
Member

oxinabox commented Jan 4, 2020

@mattBrzezinski is this a thing you might have time/interest in doing?

@mattBrzezinski
Copy link
Member

I should have some free time for this in the next while.

@mattBrzezinski mattBrzezinski self-assigned this Jan 4, 2020
@mattBrzezinski
Copy link
Member

Some quick thoughts on this, the second option seems like the most simple route to go down, and it would be nice and clean to just add this new package as a test dependency wherever you might need it.

The third option of a submodule in FDM seems a bit strange to me, as I don't see how the two are directly connected? If we did a new package, are there other utilities in our differential tools that can be pulled out and used across the board?

If so, it would be nice to have a JuliaDiff/DiffTestingUtils package which could encapsulate all sorts of generic tools and introduce this package as a test dependency in other packages.

@oxinabox
Copy link
Member

oxinabox commented Jan 4, 2020

I think this should be a new package.
Maybe TestingDiffs.jl or DiffTest.jl

I think it would be good to have this as a test dependency, here and also in other places,
like when ever someone implements ChainRules they should be able to depend on this and test it.

According to @willtebbutt there are many copies of similar code to this, in his repos and also in Nabla.

I think we should make it easiest to test ChainRules, so it should have a ChainRulesCore dependency.
But I think it also might be nice if some of it was a bit more generic, so that it can be used to test orther things, like ZygoteRules (which will be around for a bit, since we don't plan on solving mutation any time soon).
But that is lower priority can can come in a follow on PR after we create it

@willtebbutt
Copy link
Member

Separate package sounds good to me.

@nickrobinson251
Copy link
Contributor Author

What a great thread! Time allowing, very happy to help :)

@oxinabox
Copy link
Member

oxinabox commented Jan 5, 2020

I ha e created an empty JuliaDiff/DiffTest.jl repo

@mattBrzezinski
Copy link
Member

Would a new repo be most appropriate? Or fill into https://github.com/JuliaDiff/DiffTests.jl ?

If not, these package names are very very similar probably should pick something different.

@willtebbutt
Copy link
Member

Good point. They probably shouldn't be in the same repo as the intentions of the packages are somewhat different. Jarrett wrote DiffTests many moons ago to constitute a series of quite challenging problems for an AD system to get right (e.g. does a forwards-mode package successfully deal with problems where perturbation confusion could be a problem), whereas we're writing a tool for checking that an AD tool / frule / rrule returns the correct thing.

I think your idea of having different names is probably a good move. We should also reference each package in their respective readmes, explaining what the relationship between the two is to avoid confusion.

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Jan 7, 2020

AutoDiffTests might be a better name for the existing package. But even then the names are quite similar. AutoDiffChallenges and DiffTest?

Alternative we could name the new one something like ChainRulesTest / ChainRulesTestUtils. Might be a better way to go.

Naming things is difficult.
(I'm starting to think one package would be fine 😆)

@oxinabox
Copy link
Member

oxinabox commented Jan 8, 2020

How about a more different name.
The operation we are doing is basically Grad Check
but not just on gradients.

Maybe something super verbose: CheckMyDerviativesAreCorrect.jl

ChainRulesTest is fine though.

@mattBrzezinski
Copy link
Member

I think ChainRulesTests.jl is a good name, it's explicit and unique enough. I don't have the perms to make it repo, if someone else could that'd be great.

@willtebbutt
Copy link
Member

I think ChainRulesTests.jl is a good name, it's explicit and unique enough. I don't have the perms to make it repo, if someone else could that'd be great.

Have modified your permissions :)

@nickrobinson251
Copy link
Contributor Author

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

No branches or pull requests

4 participants