with_lengthscale #335
with_lengthscale #335
Conversation
Co-authored-by: willtebbutt <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| with_lengthscale(kernel::Kernel, lengthscale::Real) | ||
| with_lengthscale(kernel::Kernel, lengthscales::AbstractVector{<:Real}) |
There was a problem hiding this comment.
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.
| function Base.isequal(k::TransformedKernel, k2::TransformedKernel) | ||
| return isequal(k.kernel, k2.kernel) && isequal(k.transform, k2.transform) | ||
| end |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ==).
There was a problem hiding this comment.
please take over the PR then, this was how far I was prepared to take it:)
There was a problem hiding this comment.
I opened #336, I guess it is cleaner to just not define isequal in this PR.
Co-authored-by: willtebbutt <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: ST John <[email protected]>
|
Fixed by #336. |
To allow for a more natural construction of kernels. Provides the solution discussed in #217 (comment) and resolves #326 (the other half of #326 is a duplicate of #107...)