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

Bring over tests from ChainRulesCore. And report possible bug. #1228

Closed
wants to merge 2 commits into from

Conversation

oxinabox
Copy link
Member

We previously had these tests in ChainRulesCore (@mcabbott wrote them)
They stated breaking in julia 1.9, and some debugging determined that it was because StaticArray.jl has changed which method is hit. (This is good)
They test the behavour as it was for without the ProjectTo overload, just hitting our generic functionality.

They now error. because of a change (which might be a correct change) which we can't fix in CRC so I think these tests belong here

The failure boils down to:

julia> using StaticArrays, Test, ChainRulesCore

julia> pstat = ProjectTo(SA[1, 2, 3])
ProjectTo{SArray}(element = ProjectTo{Float64}(), axes = Size(3,))

julia> @test axes(pstat(rand(3))) === (SOneTo(3),)
Test Passed

julia> pst = ProjectTo(transpose(SA[1, 2, 3]))
ProjectTo{LinearAlgebra.Transpose}(parent = ProjectTo{SArray}(element = ProjectTo{Float64}(), axes = Size(3,)),)

julia> pst(rand(1,3))
ERROR: MethodError: no method matching iterate(::Size{(3,)})
  ...
Stacktrace:
 [1] (::ProjectTo{LinearAlgebra.Transpose, @NamedTuple{parent::ProjectTo{SArray, @NamedTuple{element::ProjectTo{…}, axes::Size{…}}}}})(dx::Matrix{Float64})
   @ ChainRulesCore ~/.julia/packages/ChainRulesCore/zoCjl/src/projection.jl:425
 [2] top-level scope
   @ REPL[15]:1
Some type information was truncated. Use `show(err)` to see complete types.

I kind of think this particular function all should work: projecting a 1 row Matrix onto a Transpose{StaticVector{...}} makes perfect sense. It is the kinda thing that comes up relatively often when writing rules and the adjoint type hasn't been written with care -- which is something ProjectTo is often used to fix.
And it did used to work.

I believe the difference is that ProjectTo{StaticArray} no longer stores axes in away that works with ProjectTo{Transpose}'s expectation: https://github.com/JuliaDiff/ChainRulesCore.jl/blob/main/src/projection.jl#L424
We really need to document how to implement an ProjectTo and what the requirements are.
E.g. here seems to assume if its an array we always store the axes in a .axes field and that that field can be indexed into.
(Help working this out and documenting it would be appreciated)

without the extension loaded:

julia> ProjectTo(SA[1,2,3]).axes
(SOneTo(3),)

vs with the extension loaded:

julia> ProjectTo(SA[1,2,3]).axes
Size(3,)

I think that is the key change. though i might not have it quite right.

@avik-pal can I leave this PR with you to decide what behavior is correct and to make sure it is tested appropriately?

test/chainrules.jl Outdated Show resolved Hide resolved
@mcabbott
Copy link
Collaborator

I believe the difference is that ProjectTo{StaticArray} no longer stores axes in away that works with ProjectTo{Transpose}'s expectation: https://github.com/JuliaDiff/ChainRulesCore.jl/blob/main/src/projection.jl#L424
We really need to document how to implement an ProjectTo and what the requirements are.
E.g. here seems to assume if its an array we always store the axes in a .axes field and that that field can be indexed into.

That seems right. I'd forgotten that Transpose did that.

On the original PR I suggested that using .axes with a different meaning may cause problems, here: #1068 (comment) . But this was ignored in the later PR #1224 which was merged.

Certainly this thing is under-documented, sorry. (Maybe all of CR is over-engineered and under-documented.)

Co-authored-by: Yuto Horikawa <[email protected]>
@oxinabox
Copy link
Member Author

Closing infavor of #1231

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