Skip to content

with_lengthscale #335

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

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/src/transform.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,9 @@ SelectTransform
ChainTransform
PeriodicTransform
```

## Convenience functions

```@docs
with_lengthscale
```
5 changes: 5 additions & 0 deletions docs/src/userguide.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ For example, a squared exponential kernel is created by
k = SqExponentialKernel() ∘ ScaleTransform(2.0)
k = compose(SqExponentialKernel(), ScaleTransform(2.0))
```
You can also use the convenience function [`with_lengthscale`](@ref):
```julia
k = with_lengthscale(SqExponentialKernel(), 0.5)
```
is equivalent to the manual composition. [`with_lengthscale`](@ref) also works with a vector-valued lengthscales argument for ARD.
Check the [Input Transforms](@ref input_transforms) page for more details.

!!! tip "How do I set the kernel variance?"
Expand Down
2 changes: 2 additions & 0 deletions src/KernelFunctions.jl
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export Transform,
IdentityTransform,
FunctionTransform,
PeriodicTransform
export with_lengthscale

export NystromFact, nystrom

Expand Down Expand Up @@ -75,6 +76,7 @@ include(joinpath("transform", "selecttransform.jl"))
include(joinpath("transform", "chaintransform.jl"))
include(joinpath("transform", "periodic_transform.jl"))
include(joinpath("kernels", "transformedkernel.jl"))
include(joinpath("transform", "with_lengthscale.jl"))

include(joinpath("basekernels", "constant.jl"))
include(joinpath("basekernels", "cosine.jl"))
Expand Down
4 changes: 4 additions & 0 deletions src/kernels/transformedkernel.jl
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ Base.:∘(k::TransformedKernel, t::Transform) = TransformedKernel(k.kernel, k.tr
Base.:∘(k::Kernel, ::IdentityTransform) = k
Base.:∘(k::TransformedKernel, ::IdentityTransform) = k

function Base.isequal(k::TransformedKernel, k2::TransformedKernel)
return isequal(k.kernel, k2.kernel) && isequal(k.transform, k2.transform)
end
Comment on lines +75 to +77
Copy link
Member

Choose a reason for hiding this comment

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

If one defines isequal one has to define hash as well since otherwise the two are not consistent. In general, if one does not care about floating point comparisons one should just define == instead of isequal since isequal falls back to ==. Here it probably would make sense to define both since it would lead to different comparisons of the kernel parameters and parameters of the transform.

Basically, most often one wants the recursive implementation in JuliaServices/AutoHashEquals.jl#18.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't follow why one cannot define isequal without defining hash; it can be easy to check containers for equality without being able to construct a hash function. Based on the docstrings of isequal and ==, it seems like the former is the one to use for containers. But in any case that seems out of scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I don't follow why one cannot define isequal without defining hash; it can be easy to check containers for equality without being able to construct a hash function.

It would lead to an inconsistency. It is explained in the docstring of isequal (and a bit shorter in the one for ==):

  isequal is the comparison function used by hash tables (Dict). isequal(x,y) must imply that hash(x) == hash(y).

  This typically means that types for which a custom == or isequal method exists must implement a corresponding hash method (and vice versa). Collections typically implement
  isequal by calling isequal recursively on all contents.

Based on the docstrings of isequal and ==, it seems like the former is the one to use for containers.

This is not correct, e.g., both are defined for arrays: https://github.com/JuliaLang/julia/blob/d85d8a91b79021668a855ab496ff3d79c2de5f4b/base/abstractarray.jl#L1842 and https://github.com/JuliaLang/julia/blob/d85d8a91b79021668a855ab496ff3d79c2de5f4b/base/abstractarray.jl#L1818 The difference is the floating point comparisons, and hence for anything involving floating point numbers it makes sense to define both.

But in any case that seems out of scope of this PR.

Just defining isequal leads to incorrect behaviour for dictionaries, so at least one should add hash as well in this PR. And then I guess one can just define == as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just defining isequal leads to incorrect behaviour for dictionaries,

could you give an example of what you mean ?

so at least one should add hash as well in this PR. And then I guess one can just define == as well?

There were already plenty of isequal implementations in the codebase; it was just the one for TransformedKernel that was missing to be able to write the doctests. I think "add hash for all of them" is beyond the scope of this PR. If you don't want this PR merged without adding all of those, please feel free to take over and do the work.

Copy link
Member

Choose a reason for hiding this comment

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

could you give an example of what you mean ?

Dictionaries use the hash function to search for keys but not isequal. Thus if the hash function is not consistent with isequal, objects that are equal according to isequal can be considered as different by the hash function, and hence the same key could appear multiple times. And vice versa, objects might seem to be equal according to hash and their values are replaced even though actually they are different according to isequal.

There were already plenty of isequal implementations in the codebase; it was just the one for TransformedKernel that was missing to be able to write the doctests. I think "add hash for all of them" is beyond the scope of this PR. If you don't want this PR merged without adding all of those, please feel free to take over and do the work.

Existing implementations should definitely not be fixed in this PR. However, I think we should not add a broken/inconsistent implementation of equality checks in this PR. In my opinion, either we should not add any equality checks at all and use different tests (e.g., just check the types or the lengthscale explicitly) or we should define the equality checks correctly (i.e., add isequal, hash, and ==).

Copy link
Member Author

Choose a reason for hiding this comment

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

please take over the PR then, this was how far I was prepared to take it:)

Copy link
Member

Choose a reason for hiding this comment

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

I opened #336, I guess it is cleaner to just not define isequal in this PR.


Base.show(io::IO, κ::TransformedKernel) = printshifted(io, κ, 0)

function printshifted(io::IO, κ::TransformedKernel, shift::Int)
Expand Down
2 changes: 2 additions & 0 deletions src/transform/chaintransform.jl
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ end
set!(t::ChainTransform, θ) = set!.(t.transforms, θ)
duplicate(t::ChainTransform, θ) = ChainTransform(duplicate.(t.transforms, θ))

Base.isequal(t::ChainTransform, t2::ChainTransform) = isequal(t.transforms, t2.transforms)

Base.show(io::IO, t::ChainTransform) = printshifted(io, t, 0)

function printshifted(io::IO, t::ChainTransform, shift::Int)
Expand Down
32 changes: 32 additions & 0 deletions src/transform/with_lengthscale.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
"""
with_lengthscale(kernel::Kernel, lengthscale::Real)
with_lengthscale(kernel::Kernel, lengthscales::AbstractVector{<:Real})
Comment on lines +2 to +3
Copy link
Member

Choose a reason for hiding this comment

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

It might be a bit easier, both to read and write, if one adds separate docstrings to the two methods. They will both show up if one looks up with_lengthscale.


Construct a transformed kernel with `lengthscale`.
If a vector `lengthscales` is passed instead, construct an "ARD" kernel with different lengthscales for each dimension.

The following two ways of constructing a squared-exponential kernel with
a given lengthscale are equivalent:

```jldoctest
julia> ℓ = 2.5;

julia> isequal(SqExponentialKernel() ∘ ScaleTransform(inv(ℓ)), with_lengthscale(SqExponentialKernel(), ℓ))
true
```

and for the ARD case:

```jldoctest
julia> ℓ = [0.5, 2.5];

julia> isequal(SqExponentialKernel() ∘ ARDTransform(inv.(ℓ)), with_lengthscale(SqExponentialKernel(), ℓ))
true
```
"""
function with_lengthscale(kernel::Kernel, lengthscale::Real)
return compose(kernel, ScaleTransform(inv(lengthscale)))
end
function with_lengthscale(kernel::Kernel, lengthscales::AbstractVector{<:Real})
return compose(kernel, ARDTransform(map(inv, lengthscales)))
end