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

add nondiff rules #252

Merged
merged 8 commits into from
Sep 3, 2020
Merged

add nondiff rules #252

merged 8 commits into from
Sep 3, 2020

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented Aug 28, 2020

Requires JuliaDiff/ChainRulesCore.jl#207

@Keno 🎁
Here is several hundred methods you don't have to AD any more.

About 438 methods at last count. (though that is only counting methods added to our rules, and in many cases they are more general than the primal methods, so it probably covers a few hundred more than that)

@oxinabox
Copy link
Member Author

idk if these are worth testing.
Perhaps as long as the macro is well tested in ChainRulesCore, that is enough, and these don't need tests.
The main mistake that could be made with these is if the wrong things were included.
Which seems hard to test in a way that doesn't just have the same content as the code

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

Very nice.

I'm not of the opinion that tests would be particularly helpful here. We'll probably want to add regression tests when we find issues with these rules / decide to add extra rules, but for now I think this is completely fine.

Things from Base @nograded in Zygote but not @non_differentiabled here:

  • Base.gc_num
  • Base.time_ns
  • Channel
  • schedule
  • floor
  • ceil
  • trunc
  • round
  • div
  • eachindex
  • Base.OneTo
  • axes
  • Colon()
  • ones
  • zeros
  • one
  • zero
  • all
  • any

Things in submodules:

  • Broadcast.combine_styles
  • Broadcast.result_style

These all look broadly correct to me. Maybe we want to implement them before merging this?

src/rulesets/Base/nondiff.jl Outdated Show resolved Hide resolved
src/rulesets/Random/random.jl Show resolved Hide resolved
src/rulesets/Random/random.jl Show resolved Hide resolved
@non_differentiable write(::IO, ::Any, ::Any, ::Any)
@non_differentiable write(::IO, ::Any, ::Any, ::Any, ::Any)

@non_differentiable Libc.free(::Any)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like Libc has quite a lot of methods that certainly don't appear differentiable that don't appear on this list. Is there a particular reason for the omission, or just because you've not had time to write them out yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

We only grabbed once that had particular types uses in their type signatures.
In particular we grabbed nothing with Number in the type-sig

Copy link
Member

Choose a reason for hiding this comment

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

Oh interesting. Just looking through the methods there are definitely some good candidates e.g. calloc.

Copy link
Member

Choose a reason for hiding this comment

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

What do you want to do about this one? I'm happy to leave for future work for the sake of getting this in provided that we document what we think is missing in an issue somewhere.

@oxinabox
Copy link
Member Author

oxinabox commented Aug 28, 2020

I think the following we already have as returning Zero.

ones
zeros
one
zero

I am dubious about these:

  • floor
  • ceil
  • trunc
  • round
  • div

We should definately have these:

  • eachindex
  • Base.OneTo
  • axes
  • Colon()
  • all

I think these make sense to have

  • Broadcast.combine_styles
  • Broadcast.result_style

Also i guess the other logical operators

  • any
  • !
  • xor

@willtebbutt
Copy link
Member

I am dubious about these:

Hmmm yeah, maybe. There are definitely some methods of them that involve clearly non-differentiable types that we can be pretty sure are @non_differentiable.

Assuming that no one is punning on them, I think I'm correct in saying that they all have zero derivative everywhere, except at the points where the functions change value. At those points the derivative is unbounded / undefined / whatever we're calling 1 / h as h -> 0. So my opinion is that it's probably fine to @non_differentiable them.

@oxinabox
Copy link
Member Author

Right but @non_differentiable does DoesNotExist not Zero()

@willtebbutt
Copy link
Member

Good point.

@oxinabox
Copy link
Member Author

perhaps we can generalize the code.
Maybe in a follow up, or maybe now.

Could be something like @constant_rule f(x,y) --> DoesNotExist() @costant_rule f(x,y) --> Zero()

@willtebbutt
Copy link
Member

Good point -- a constant rule would be helpful.

I vote that we get this in and then add a @constant_rule construct of some kind.

oxinabox and others added 3 commits September 1, 2020 14:28
Co-authored-by: willtebbutt <wt0881@my.bristol.ac.uk>
Co-authored-by: willtebbutt <wt0881@my.bristol.ac.uk>
@oxinabox
Copy link
Member Author

oxinabox commented Sep 2, 2020

Other zyogte ones:

  • Base.gc_num
  • Base.time_ns
  • Channel
  • schedule

@oxinabox
Copy link
Member Author

oxinabox commented Sep 3, 2020

There are lots more we could add, but lets add them as they come up.

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

LGTM!

@oxinabox oxinabox merged commit 283f329 into master Sep 3, 2020
bors bot added a commit to FluxML/Zygote.jl that referenced this pull request Sep 3, 2020
780: Move a bunch of no_grad to ChainRules r=oxinabox a=oxinabox

this is the partner to JuliaDiff/ChainRules.jl#252
It will fail til that is merged and tagged

What is left is:

- Types (because JuliaDiff/ChainRulesCore.jl#213) (e.g. `Colon`, `OneTo` `Channel`)
- Things to which the derivative is `Zero()` not `DoesNotExist()` (e.g. `one`, `ones`, `zero`, `zeros`)
- Things that felt too magic: e.g. `Base.eval`


Should I bump patch version and tag a release?

Co-authored-by: Lyndon White <lyndon.white@invenialabs.co.uk>
Co-authored-by: Lyndon White <oxinabox@ucc.asn.au>
bors bot added a commit to FluxML/Zygote.jl that referenced this pull request Sep 4, 2020
780: Move a bunch of no_grad to ChainRules r=oxinabox a=oxinabox

this is the partner to JuliaDiff/ChainRules.jl#252
It will fail til that is merged and tagged

What is left is:

- Types (because JuliaDiff/ChainRulesCore.jl#213) (e.g. `Colon`, `OneTo` `Channel`)
- Things to which the derivative is `Zero()` not `DoesNotExist()` (e.g. `one`, `ones`, `zero`, `zeros`)
- Things that felt too magic: e.g. `Base.eval`


Should I bump patch version and tag a release?

Co-authored-by: Lyndon White <lyndon.white@invenialabs.co.uk>
Co-authored-by: Lyndon White <oxinabox@ucc.asn.au>
@oxinabox oxinabox deleted the ox/nondiff branch October 16, 2020 14:47
@mcabbott mcabbott mentioned this pull request Sep 1, 2021
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.

3 participants