Skip to content

Fixes #115 #123

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Jun 24, 2020
Merged

Fixes #115 #123

merged 11 commits into from
Jun 24, 2020

Conversation

theogf
Copy link
Member

@theogf theogf commented Jun 19, 2020 •
edited
Loading

This PR is the answer for #115 , and also solve another issue with mapslices (used in FunctionTransform) to replace it by slicemap from SliceMap.jl. Allowing AD for Zygote.

@devmotion
Copy link
Member

Could we define our own KernelFunctions.pairwise and KernelFunctions.pairwise! instead, as suggested in #115 (comment), to avoid adding more type piracy?

I didn't know about SliceMap - is slicemap only used because it provides an adjoint for Zygote or is it also more efficient than mapslices? SliceMap adds indirect dependencies on Tracker and ForwardDiff, I'm not sure if that's an issue.

@willtebbutt
Copy link
Member

SliceMap adds indirect dependencies on Tracker and ForwardDiff, I'm not sure if that's an issue.

This is an issue. These are not dependencies that I would want to bake into this package. We should be aiming for ChainRulesCore only.

@theogf theogf changed the title Fixes #115 and add SliceMap Fixes #115 ~~and add SliceMap~~ Jun 22, 2020
@theogf theogf changed the title Fixes #115 ~~and add SliceMap~~ Fixes #115 Jun 22, 2020
@theogf theogf requested review from willtebbutt and devmotion June 23, 2020 20:23
willtebbutt
willtebbutt approved these changes Jun 24, 2020
View reviewed changes
Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@devmotion any thoughts?

devmotion
devmotion approved these changes Jun 24, 2020
View reviewed changes
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

theogf and others added 2 commits June 24, 2020 13:41
@theogf theogf merged commit b6876a1 into JuliaGaussianProcesses:master Jun 24, 2020
@theogf theogf deleted the fix_transform branch August 4, 2020 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devmotion devmotion devmotion approved these changes

@willtebbutt willtebbutt willtebbutt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@theogf @devmotion @willtebbutt