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

extern(::Casted) seems ill-defined. (Remove Casted?) #10

Closed
willtebbutt opened this issue Apr 13, 2019 · 5 comments · Fixed by #55
Closed

extern(::Casted) seems ill-defined. (Remove Casted?) #10

willtebbutt opened this issue Apr 13, 2019 · 5 comments · Fixed by #55

Comments

@willtebbutt
Copy link
Member

extern(cast(1)) returns 1. This is unfortunate behaviour as we loose all of the e.g. size information that (arguably) should be associated with the casted thing. For example the rrule for sum currently returns a casted 1 to represent an array of ones, which would have been the same size as the input. Consequently, without reference to the input you don't know what the appropriate size is for a particular Casted object.

Is there a particular reason that this route has been taken as opposed to, say, returning AbstractFills (from FillArrays.jl) which would be just as efficient in all cases that I can think of.

@jrevels
Copy link
Member

jrevels commented Apr 14, 2019

Is there a particular reason that this route has been taken as opposed to, say, returning AbstractFills (from FillArrays.jl) which would be just as efficient in all cases that I can think of.

Mainly because I didn't know about FillArrays.jl, seems like a good idea to utilize that instead 🙂

@willtebbutt
Copy link
Member Author

What do you reckon is the best way forwards? I guess we could either:

  • Keep the Casted differential and just re-implement using FillArrays, or
  • Get rid of the Casted differential and re-implement using FillArrays.

Any preference / have I missed an obvious option @jrevels ?

@jrevels
Copy link
Member

jrevels commented Apr 23, 2019

Get rid of the Casted differential and re-implement using FillArrays.

This would be preferable, if possible, though I'm not sure if FillArrays covers everything that Casted covers.

Casted basically is meant to be Base.Broadcast.Broadcasted with a bit of extra behavior (the baked in * and + definitions since it's an AbstractDifferential). We could've defined that extra behavior directly on Base.Broadcast.Broadcasted, but of course that would be type piracy and can lead to some issues (even if the type piracy was hidden under a Cassette context).

Example where cast is more like broadcasted:

frule(::typeof(BLAS.asum), x) = (BLAS.asum(x), Rule(Δx -> sum(cast(sign, x) * Δx)))

rrule(::typeof(BLAS.asum), x) = (BLAS.asum(x), Rule(ΔΩ -> ΔΩ * cast(sign, x)))

Even if FillArrays doesn't support this particular use case, it'd be great to stop punning on Casted for cases where formal (i.e. tested against) support for FillArrays would suffice.

@willtebbutt
Copy link
Member Author

Hmm yeah, this is interesting. Your above examples definitely aren't something that you could straightforwardly re-implement using FillArrays, so I agree that it might just be best to introduce FillArrays in rules when it makes sense and to keep the cast stuff when it doesn't.

@oxinabox oxinabox transferred this issue from JuliaDiff/ChainRules.jl Aug 2, 2019
@oxinabox
Copy link
Member

oxinabox commented Aug 7, 2019

From #18

Casted

It is kind of the generalization of One, and Zero.
(in that One() could also be written Casted(true) etc).
It lets us lazily delay computing a broadcast,
so that it can be fused later.
But I think in the short term we can simplify the code
by replacing say
Rule((Δx, Δy) -> sum(Δx * cast(y)) + sum(cast(x) * Δy))
with
Rule((Δx, Δy) -> sum(Δx .*y) + sum(x .* Δy)
(from here
which for that particular case would even be identical in performance I think.
Since it does not end up returning any kind of lazy computation.
And later we can try getting back the lazy computation and broadcast fusing by returning broadcasted.

So we can just get ride of it for now.

@oxinabox oxinabox changed the title extern(::Casted) seems ill-defined extern(::Casted) seems ill-defined. (Remove Casted?) Aug 7, 2019
@oxinabox oxinabox mentioned this issue Oct 17, 2019
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 a pull request may close this issue.

3 participants