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 ProjectTo(::Any) = identity #458

Merged
merged 2 commits into from
Sep 21, 2021
Merged

Conversation

mcabbott
Copy link
Member

Closes #442, I think.

Besides ProjectTo(::Any) = identity, this also adds a few methods marking things like Symbol, Char as non-differentiable. Could also just leave them alone.

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2021

Codecov Report

Merging #458 (1fb088a) into master (47389f5) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
+ Coverage   92.94%   93.02%   +0.07%     
==========================================
  Files          14       14              
  Lines         794      803       +9     
==========================================
+ Hits          738      747       +9     
  Misses         56       56              
Impacted Files Coverage Δ
src/projection.jl 98.12% <100.00%> (+0.02%) ⬆️
src/rule_definition_tools.jl 96.17% <0.00%> (+0.02%) ⬆️
src/differentials/thunks.jl 95.00% <0.00%> (+0.10%) ⬆️
src/differentials/composite.jl 83.19% <0.00%> (+0.43%) ⬆️

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 47389f5...1fb088a. Read the comment docs.

@mcabbott
Copy link
Member Author

Bump?

@oxinabox
Copy link
Member

I would like more voices on #442 before I merge

src/projection.jl Outdated Show resolved Hide resolved
@@ -143,6 +144,11 @@ ProjectTo{P}(::NamedTuple{T, <:Tuple{_PZ, Vararg{<:_PZ}}}) where {P,T} = Project
# Bool
ProjectTo(::Bool) = ProjectTo{NoTangent}() # same projector as ProjectTo(::AbstractZero) above
Copy link
Member

Choose a reason for hiding this comment

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

Combine with below?

src/projection.jl Outdated Show resolved Hide resolved
src/projection.jl Outdated Show resolved Hide resolved
Co-authored-by: Lyndon White <[email protected]>
Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

Merge when happy.

@mcabbott
Copy link
Member Author

Great let's do this.

@mcabbott mcabbott merged commit 0e560c6 into JuliaDiff:master Sep 21, 2021
@mcabbott mcabbott deleted the project_all branch September 21, 2021 15:10
@mcabbott
Copy link
Member Author

Oh no the version is in fact wrong. Mind if I commit it straight to master?

@mzgubic
Copy link
Member

mzgubic commented Sep 21, 2021

yeah that's fine

@mcabbott
Copy link
Member Author

FluxML/Zygote.jl#1044 has been updated to use this, and simplified. It needs someone's approval. Seems uncontroversial & would be nice to get in before it goes stale.

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.

Allow ProjectTo on non-differential types?
4 participants