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 rules for specialized norm functions and normalize #226

Merged
merged 70 commits into from
Dec 6, 2020

Conversation

sethaxen
Copy link
Member

@sethaxen sethaxen commented Jul 6, 2020

This PR adds frules (edit: it only adds the frule for the scalar function and norm without p) and rrules of the various generic norm functions (and friends) in LinearAlgebra:

  • norm
  • norm2
  • norm1
  • normInf
  • normMinusInf
  • normp
  • normalize
  • normalize! (frule only)

The frules are patterned after the primal functions and work on iterators. The rrules are restrict to AbstractArray primal inputs because I don't see how to generically write rrules for iterators. This PR replaces the current rrule for norm to dispatch to the other rules. The rules use the subgradient convention throughout. I'm still working on the test suite.

This PR will fix #204 and FluxML/Zygote.jl#663.

@sethaxen
Copy link
Member Author

sethaxen commented Nov 23, 2020

This PR is now ready for review. A few notes:

  • I removed almost all of the frules. In almost every case, I was just hardcoding what a sane AD should do, and because the primal and frule were so intertwined, it would be annoying to maintain. I did keep the frule for norm(x) and norm2(x) though, because there the efficient implementation does not fuse with the primal.
  • This PR depends on Add tests and fixes for to_vec type-stability FiniteDifferences.jl#118.
  • CI won't run until Add GitHub Actions CI workflow #309 is merged
    Edit:
  • Should this be a breaking change, since it restricts the type of the rrule for norm compared to before the PR?

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.

I'm approving subject to the things that you note above being resolved as I don't want to hold you back, and deciding whether to release a new minor version (if we decide to do that, then we should probably -DEV briefly, and bundle it in with #311 to avoid making multiple breaking releases.

I've made some style suggestions to make the type unions more readable.

We should discuss whether or not it's a breaking change though. Before, would you have expected things to break if someone passed in a type outside of the new constraints?

src/rulesets/LinearAlgebra/norm.jl Outdated Show resolved Hide resolved
src/rulesets/LinearAlgebra/norm.jl Outdated Show resolved Hide resolved
src/rulesets/LinearAlgebra/norm.jl Outdated Show resolved Hide resolved
src/rulesets/LinearAlgebra/norm.jl Outdated Show resolved Hide resolved
src/rulesets/LinearAlgebra/norm.jl Outdated Show resolved Hide resolved
Co-authored-by: willtebbutt <wt0881@my.bristol.ac.uk>
@sethaxen
Copy link
Member Author

We should discuss whether or not it's a breaking change though. Before, would you have expected things to break if someone passed in a type outside of the new constraints?

Not necessarily. The rule should have worked for any array type whose differential could be represented by scaling the array via broadcast. So it would have worked for Hermitian/Symmetric as well as StaticArrays, but would probably have failed for others, in particular for arrays like UnitLowerTriangular, where the diagonal of the differential should be zero but would not have been.

@willtebbutt
Copy link
Member

Hmmm interesting. So it probably fixes some potential bugs, but might remove some functionality. @nickrobinson251 any thoughts on the correct course of action here?

@sethaxen
Copy link
Member Author

sethaxen commented Dec 3, 2020

@nickrobinson251 what are your thoughts on the version number here?

@codecov-io
Copy link

codecov-io commented Dec 3, 2020

Codecov Report

Merging #226 (12b460e) into master (a91009c) will decrease coverage by 0.28%.
The diff coverage is 95.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
- Coverage   97.15%   96.87%   -0.29%     
==========================================
  Files          16       17       +1     
  Lines         880      993     +113     
==========================================
+ Hits          855      962     +107     
- Misses         25       31       +6     
Impacted Files Coverage Δ
src/ChainRules.jl 100.00% <ø> (ø)
src/rulesets/LinearAlgebra/dense.jl 98.07% <ø> (-0.20%) ⬇️
src/rulesets/LinearAlgebra/norm.jl 95.20% <95.20%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a91009c...12b460e. Read the comment docs.

@sethaxen
Copy link
Member Author

sethaxen commented Dec 5, 2020

Okay, this PR should be 100% complete, just pending input on choice of version number.

@nickrobinson251
Copy link
Contributor

I'd say this is a bug fix 👍
Thanks a lot!

@oxinabox
Copy link
Member

oxinabox commented Dec 6, 2020

Agreed, bug-fixes are never considered breaking.
Plus, this didn't break Zygote's tests, and I know it won't break ReversePropagation's Tests sice that only uses our scalar rules.
I am pretty sure it won't break Nabla.

Also it's much easier to deal with something incorrectly called non-breaking (revert, tag a patch, unrevert tag a breaking).
There isn't really any way to undo something bring considered breaking when it wasn't

@sethaxen
Copy link
Member Author

sethaxen commented Dec 6, 2020

Great! Thanks to you both!

@sethaxen sethaxen merged commit fa4b93a into JuliaDiff:master Dec 6, 2020
@sethaxen sethaxen deleted the intnorms branch December 6, 2020 11:54
@nickrobinson251 nickrobinson251 added the type constraints Potentially raises a question about how tightly to constrain argument types for a rule. See #232 label Jan 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type constraints Potentially raises a question about how tightly to constrain argument types for a rule. See #232
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add rrule for infinite-norm
6 participants