Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

scatter cleanup and fix min/max #319

Merged
merged 6 commits into from
May 28, 2021
Merged

scatter cleanup and fix min/max #319

merged 6 commits into from
May 28, 2021

Conversation

CarloLucibello
Copy link
Member

@CarloLucibello CarloLucibello commented May 24, 2021

Fix #317. The main problem was in the use of FiniteDifferences' central difference method on singularities of non-smooth functions.

@CarloLucibello CarloLucibello requested a review from yuehhua May 24, 2021 12:09
@yuehhua
Copy link
Member

yuehhua commented May 24, 2021

I am quite curious about the semantic difference between DoesNotExist and NoTangent.

@CarloLucibello
Copy link
Member Author

I am quite curious about the semantic difference between DoesNotExist and NoTangent.

no difference, just a recent renaming in ChainRulesCore.jl

@yuehhua
Copy link
Member

yuehhua commented May 24, 2021

Maybe we could come out with a new name for _check_dims? Maybe _check_scatter_dims or something else?

@yuehhua
Copy link
Member

yuehhua commented May 24, 2021

For scatter, I have been used to write something like this.

scatter(op,
        src::AbstractArray{Tsrc,Nsrc},
        idx::AbstractArray{Tidx,Nidx};
        init = scatter_empty(op, Tsrc))

I think it can avoid branching on isnothing.

@yuehhua
Copy link
Member

yuehhua commented May 24, 2021

The dimension checking is slightly different for scatter and gather. scatter calls _check_dims/scatter_dims with (dst, src, idx), while, reversely, gather calls _check_dims/scatter_dims with (src, dst, idx). For abstraction, I give _check_dims/scatter_dims a signature of (X, Y, idx), so they are both available for scatter and gather.

@yuehhua
Copy link
Member

yuehhua commented May 26, 2021

I am good with this PR.

@CarloLucibello
Copy link
Member Author

buildkite fail is unrelated, merging

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add rrules for max/min scatter
2 participants