-
Notifications
You must be signed in to change notification settings - Fork 39
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
Closed
with_lengthscale #335
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
1ae1b26
with_lengthscale
st-- 740613c
add Base.== for ScaleTransform and ARDTransform
st-- a2f3d3f
Apply suggestions from code review
st-- 7d23f52
Apply suggestions from code review
st-- 239cee5
Merge branch 'master' into st/with_lengthscale
st-- 2af102b
Revert "add Base.== for ScaleTransform and ARDTransform"
st-- 9ef16ee
add missing isequal definitions
st-- 912c6cc
isequal instead of == for containers
st-- 2e06b71
map(inv, instead of inv.(
st-- f6c5d28
imperative
st-- c74e739
add with_lengthscale to docs
st-- File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,3 +36,9 @@ SelectTransform | |
ChainTransform | ||
PeriodicTransform | ||
``` | ||
|
||
## Convenience functions | ||
|
||
```@docs | ||
with_lengthscale | ||
``` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
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 |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 definehash
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 ofisequal
sinceisequal
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.
There was a problem hiding this comment.
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 defininghash
; it can be easy to check containers for equality without being able to construct a hash function. Based on the docstrings ofisequal
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would lead to an inconsistency. It is explained in the docstring of
isequal
(and a bit shorter in the one for==
):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.
Just defining
isequal
leads to incorrect behaviour for dictionaries, so at least one should addhash
as well in this PR. And then I guess one can just define==
as well?There was a problem hiding this comment.
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 ?
There were already plenty of
isequal
implementations in the codebase; it was just the one forTransformedKernel
that was missing to be able to write the doctests. I think "addhash
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dictionaries use the
hash
function to search for keys but notisequal
. Thus if thehash
function is not consistent withisequal
, objects that are equal according toisequal
can be considered as different by thehash
function, and hence the same key could appear multiple times. And vice versa, objects might seem to be equal according tohash
and their values are replaced even though actually they are different according toisequal
.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==
).There was a problem hiding this comment.
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:)
There was a problem hiding this comment.
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.