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

WIP: preproject and project implementation #383

Closed
wants to merge 21 commits into from
Closed

Conversation

mzgubic
Copy link
Member

@mzgubic mzgubic commented Jun 25, 2021

Alternative to #380 and #382. See usage in JuliaDiff/ChainRules.jl#457. Uses @mcabbott's preproject(x) idea (somewhere I can't find) instead of a closure. This is cool because it allows the project(T, dx) to be extended depending on both the

Some observations (comments welcome):

  • This PR is cool because similarly to WIP: projector implementation (returning a closure) #382 it allows us to not close over primal value (x)
  • This PR is cool because unlike WIP: projector implementation (returning a closure) #382, where the returned closure can not be extended, the project function can be extended by someone developing a Quaternions package. They would simply define project(::Type{Real}, dx::Quaternion) = ...
  • It's also relatively clean (only one- and two-argument functions), compared to WIP: project implementation #380 where there are 3-arg functions. On the other hand calling project now also needs an info kwarg which stores the information about the primal x, such as size or element type.
  • These also seem to compose relatively naturally. I.e. preproject returns a nested NamedTuple of required info. Similarly the project can just peel off the necessary info and pass it on to the inner project call.

There are still some things I need to address from @mcabbott's comments on #382, but just putting it out here for discussion.

IMO this is the best way forward of the three PRs. Thoughts?

@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2021

Codecov Report

Merging #383 (d8848f5) into master (4b1a2f6) will increase coverage by 0.60%.
The diff coverage is 97.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #383      +/-   ##
==========================================
+ Coverage   89.12%   89.73%   +0.60%     
==========================================
  Files          14       15       +1     
  Lines         561      604      +43     
==========================================
+ Hits          500      542      +42     
- Misses         61       62       +1     
Impacted Files Coverage Δ
src/ChainRulesCore.jl 100.00% <ø> (ø)
src/projection.jl 97.43% <97.43%> (ø)
src/differentials/thunks.jl 94.89% <0.00%> (+0.21%) ⬆️

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 4b1a2f6...d8848f5. Read the comment docs.

@mzgubic
Copy link
Member Author

mzgubic commented Jul 6, 2021

closed in favour of #385

@mzgubic mzgubic closed this Jul 6, 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