Skip to content

Fixing implicit gradients #141

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 4 commits into from
Jul 31, 2020
Merged

Fixing implicit gradients #141

merged 4 commits into from
Jul 31, 2020

Conversation

theogf
Copy link
Member

@theogf theogf commented Jul 26, 2020 •
edited
Loading

Until now there was a major issue where implicit gradients on TransformedKernel, i.e. :

k = transform(SqExponentialKernel(), 2.0)
gradient(params(k) do 
 something...
end

would return nothing.
This fixes #137 by not using map to apply the transform operation and adds tests to check the behaviour

@theogf theogf requested a review from willtebbutt July 26, 2020 16:12
@theogf
Copy link
Member Author

theogf commented Jul 27, 2020

@willtebbutt @sharanry This correction is very important for me (AGP.jl relies completely on it). So I would merge it and release a new breaking release tonight.
Btw, I believe JuliaGaussianProcesses/AbstractGPs.jl#14 could be also solved by using another function then Base.map.

@willtebbutt
Copy link
Member

@willtebbutt @sharanry This correction is very important for me (AGP.jl relies completely on it). So I would merge it and release a new breaking release tonight.

I'll try and take a look at this this evening.

willtebbutt
willtebbutt reviewed Jul 27, 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.

Just a quick question.

src/transform/scaletransform.jl Outdated
Comment on lines 20 to 24
(t::ScaleTransform)(x) = first(t.s) * x

_map(t::ScaleTransform, x::AbstractVector{<:Real}) = first(t.s) .* x
_map(t::ScaleTransform, x::ColVecs) = ColVecs(first(t.s) .* x.X)
_map(t::ScaleTransform, x::RowVecs) = RowVecs(first(t.s) .* x.X)
_map(t::ScaleTransform, x::AbstractVector{<:Real}) = t(x)
_map(t::ScaleTransform, x::ColVecs) = ColVecs(t(x.X))
_map(t::ScaleTransform, x::RowVecs) = RowVecs(t(x.X))
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why this code needs to change? It's unclear to me as it's already using the _map function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that is not necessary! Sorry for the noise. I thought it was nicer to have a more uniform approach

Copy link
Member

Choose a reason for hiding this comment

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

I think I actually prefer the way it was written previously 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I will revert it back

@theogf theogf merged commit 6aea76e into master Jul 31, 2020
@devmotion devmotion deleted the fix_implicit_gradients branch July 31, 2020 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willtebbutt willtebbutt willtebbutt left review comments

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

Successfully merging this pull request may close these issues.

Implicit gradient failing with matrices
2 participants
@theogf @willtebbutt