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: Adjoint problems #134

Merged
merged 3 commits into from
Jan 5, 2021
Merged

WIP: Adjoint problems #134

merged 3 commits into from
Jan 5, 2021

Conversation

willtebbutt
Copy link
Member

@willtebbutt willtebbutt commented Jan 5, 2021

As @sethaxen pointed out in #133, #131 accidentally disabled a number of tests, and introduced a weird bug. This PR currently resolves the first of these problems, and I'm presently working on the second.

@willtebbutt
Copy link
Member Author

willtebbutt commented Jan 5, 2021

@sethaxen could you confirm that this resolves the problem? I'm quite nervous about this because I don't really understand what's going on 😬 .

@sethaxen
Copy link
Member

sethaxen commented Jan 5, 2021

@sethaxen could you confirm that this resolves the problem? I'm quite nervous about this because I don't really understand what's going on 😬 .

Well this is bizarre. Before it was correct for Adjoint and Composite differentials but conjugated for Matrix. Now this is correct for Matrix and Adjoint differentials but is conjugated for Composite

@willtebbutt
Copy link
Member Author

Booooooooooooooooo. I wonder whether it ever consistent prior to #131 ?

@sethaxen
Copy link
Member

sethaxen commented Jan 5, 2021

Oooooh, not it wasn't. Composite has always been wrong for Adjoint. ChainRules worked around that by checking that the pullback produced the same output for Matrix and Composite cotangents but only passed the Matrix to rrule_test.
I think then this looks good to go.

@willtebbutt willtebbutt merged commit e262eb8 into master Jan 5, 2021
@willtebbutt willtebbutt deleted the wct/fix-Adjoint-vec-bug branch January 5, 2021 22:44
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.

2 participants