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

Make getproperty on Composite unthunk #121

Merged
merged 2 commits into from
Oct 15, 2020
Merged

Conversation

nickrobinson251
Copy link
Contributor

@oxinabox
Copy link
Member

I think we should hold off on this until we see as case that someone actually uses thunking inside a a Composite with more than one fields (and thus can't move it to the outside).

I suspect @willtebbutt already might have one.
So that might not be long

@nickrobinson251 nickrobinson251 added the pending-clear-need We are not certain we need this. So waiting for evidence to be presented label Jan 30, 2020
@sethaxen
Copy link
Member

The rules sincos (and sort of eigen) in JuliaDiff/ChainRules.jl#193 are an example where one would want Thunk properties of Composite. But in those cases, if a user called eigen but only used the eigenvalues or eigenvectors or called sincos but only used sin or cos, it's kind of their fault for writing wasteful primal functions.

@oxinabox
Copy link
Member

sincos in forward mode, yes?
We currently don't do any thinking in forward mode.
I don't think.

@sethaxen
Copy link
Member

Ah good point. I did thunk in forward mode in #JuliaDiff/ChainRules.jl#193, but I see I probably shouldn't.

@oxinabox
Copy link
Member

After seeing svd_rev in JuliaDiff/ChainRules.jl#282
I now thing we should do this.

While it is reasonable for an AD to unthunk the things it passes into a pullback,
if those things are Composite it shouldn't since it doesn't know which fields will need unthunking.

Can you rebase this?

@oxinabox oxinabox added Structural Tangent Related to the `Tangent` type for structured (composite) values and removed pending-clear-need We are not certain we need this. So waiting for evidence to be presented labels Oct 14, 2020
@oxinabox
Copy link
Member

oxinabox commented Oct 15, 2020

Integration test failure is from an assertion in the tests; which can just be removed. (Or replaces with something that accesses the backing directly.

We should make a follow up to remove that assertion, and maybe remove the rest of
JuliaDiff/ChainRules.jl#282
except for the tests maybe

Are you able to make that PR after this is merged?

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.

Should getindex and iterate also be changed to unthunk?
I think that would make sense since those are the otherways you can try and acces the content

@nickrobinson251
Copy link
Contributor Author

Should getindex and iterate also be changed to unthunk?
I think that would make sense since those are the otherways you can try and acces the content

I think yes. But i will open an issue / leave to a follow-up if that's okay?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Structural Tangent Related to the `Tangent` type for structured (composite) values
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getproperty on Composite should unthunk?
3 participants