Skip to content

Add ProjectTo{Tuple} #457

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

Closed
wants to merge 4 commits into from
Closed

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Sep 14, 2021

Besides actually projecting values, the nice thing about having this is that project(dx) would handle constructing the right Tangent type. This will simplify rules over x::AbstractVecOrTuple.

The other thing which would simplify such rules is a function _no_tuple_tangent which does nothing to vectors but calls backing on Tangents, so that internally you can deal with the tuple or vector on equal terms. Not yet added here, but could be.

@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2021

Codecov Report

Merging #457 (a8fad7f) into master (47389f5) will decrease coverage by 0.51%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #457      +/-   ##
==========================================
- Coverage   92.94%   92.43%   -0.52%     
==========================================
  Files          14       14              
  Lines         794      806      +12     
==========================================
+ Hits          738      745       +7     
- Misses         56       61       +5     
Impacted Files Coverage Δ
src/projection.jl 97.77% <92.85%> (-0.32%) ⬇️
src/compat.jl 0.00% <0.00%> (-55.56%) ⬇️
src/differentials/thunks.jl 95.00% <0.00%> (+0.10%) ⬆️
src/differentials/composite.jl 82.90% <0.00%> (+0.14%) ⬆️
src/rule_definition_tools.jl 96.66% <0.00%> (+0.51%) ⬆️

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...a8fad7f. Read the comment docs.

@oxinabox
Copy link
Member

The other thing which would simplify such rules is a function _no_tuple_tangent which does nothing to vectors but calls backing on Tangents, so that internally you can deal with the tuple or vector on equal terms. Not yet added here, but could be

Let's discuss in a separate issue

@oxinabox oxinabox linked an issue Sep 14, 2021 that may be closed by this pull request
@mcabbott
Copy link
Member Author

As written this will allow ProjectTo([(1,2), (3,4)]) for an array of tuples, and this will (1) store a projector per element, like an array of arrays, and (2) apply them one by one to turn the gradient into a Vector{Tangent{...}}.

It's possible that the array projector should notice this, and (1) treat a uniform array of tuples more like an array of numbers, one projector, and (2) possibly just reinterpret or something?

if all(p -> p isa ProjectTo{<:AbstractZero}, elements)
ProjectTo{NoTangent}() # short-circuit if all elements project to zero
else
return ProjectTo{Tuple}(; type=typeof(xs), elements=elements)
Copy link
Member

Choose a reason for hiding this comment

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

I kinda want to stick the whole primal type into the type parameter for ProjectTo.
We need it anyway.

This typeof is going to make accessing project.type type unstable, since it will make a NamedTuple with a DataType typed field.

julia> (;type = Type{typeof((1,2))}) |> typeof
NamedTuple{(:type,), Tuple{DataType}}

So if we want to store it as an element we need to wrap it up in Val

julia> (;type = Val{typeof((1,2))}()) |> typeof
NamedTuple{(:type,), Tuple{Val{Tuple{Int64, Int64}}}}

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Same applies to Ref.

@oxinabox
Copy link
Member

oxinabox commented Sep 21, 2021

Major comment I think we need to resolve:

I think ProjectTos are parameterized by the tangent type, not by the primal type.
So not ProjectTo{Tuple} but rather:

ProjectTo{Tangent}(; type=Val(typeof(xs)), elements=elements)

or, and I would make a strongish case for this:

ProjectTo{Tangent{typeof(xs)}(;elements=elements)

If we did that later one then there are 3 advantages:

  • we can easily let things that are already Tangent{P} of right tuple primal pass through, via dispatch.
  • we don't need to store the type as a field with val
  • It seems like it will work better when(/if) we want to extend it for structural tangents for structs and NamedTuples. And I think we will need to distinguish them because we might well want to e.g project a Tuple onto a NamedTuple or visaversa. And we might want to specialize some structural tangents for things like factorizations in nontrivial ways (not sure yet).

@mcabbott
Copy link
Member Author

Yes I guess I agree that ProjectTo{Tangent} would be a better thing. The existing code for Ref should be re-written to match; in fact perhaps they can mostly become one.

However, I'm not going to do that in this PR. It can be a fresh one, either after or instead of this one.

Being able to handle tuples at all, and produce the right Tangents, was intended to simplify JuliaDiff/ChainRules.jl#526 .

@oxinabox
Copy link
Member

oxinabox commented Sep 21, 2021

However, I'm not going to do that in this PR. It can be a fresh one, either after or instead of this one.

I think instead-of, rather than after.
Not keen to make a change we know we are going to substantially rework; unless there is a particular reason.

Being able to handle tuples at all, and produce the right Tangents, was intended to simplify JuliaDiff/ChainRules.jl#526 .

That still applies right?
That PR is able to wait til we resolve this right, no?

Or is there a rush to get that in for a thing?

@mcabbott
Copy link
Member Author

No, there's no rush. Just revisiting things to look for loose ends.

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.

ProjectTo called on tuples
3 participants