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: Change matrix to_vec semantics #187

Closed
wants to merge 2 commits into from

Conversation

willtebbutt
Copy link
Member

Relates to / resolves #132

Since ChainRulesTestUtils no longer relies on checking equality in the to_veced space (I don't believe), we're freed up to make this change.

In short: after this change, it will generally be a really bad idea to check equality in the to_veced space (I think it was probably a bad idea anyway to be honest, because it's generally a lossy transformation).

The motivation for making this PR now was that I was trying to implement some rrules, and encountered the issue that to_vec(rand_tangent(::Diagonal)) and to_vec(::Diagonal) yield different sized vectors, which causes problems with FiniteDifferences's jpvp and / or jvp functionality.

ToDo:

  • check that this doesn't break ChainRules

@willtebbutt willtebbutt changed the title RFC: Change matrix to_vec semantics WIP: Change matrix to_vec semantics Aug 1, 2021
@mzgubic
Copy link
Member

mzgubic commented Aug 2, 2021

This will very much break ChainRules unfortunately :(. I tried this recently and wrote something up here for diagonal: #186

Also, rand_tangent(::Diagonal) now returns a Diagonal (not a Tangent{Diagonal}) since rand_tangent has been moved back to ChainRulesTestUtils and ProjectTo used.

I do think though that we are missing something in our finite differencing story. One aspect is JuliaDiff/ChainRulesTestUtils.jl#199 for example. It seems like we need something similar to ProjectTo

@willtebbutt
Copy link
Member Author

Thanks for pointing out that you've already looked at this @mzgubic -- turns out I've been using an outdated ChainRulesTestUtils version anyway...

I'll close for now. I agree that there's other stuff to sort out.

@willtebbutt willtebbutt closed this Aug 2, 2021
@mzgubic
Copy link
Member

mzgubic commented Aug 2, 2021

Oh, I see - I found that the newer ChainRulesTestUtils version with project inside rand_tangent for structured arrays helps solve quite a few of the more annoying things we've been running into

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.

to_vec doesn't preserve equality
2 participants