Skip to content
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

Remove Metric <: SemiMetric <: PreMetric #228

Closed
wants to merge 8 commits into from

Conversation

matthieugomez
Copy link
Contributor

@matthieugomez matthieugomez commented Sep 12, 2021

This PR removes the type structure Metric, SemiMetric, PreMetric which makes it harder to define new distance types (solving #199)

Still, generic distances are encouraged to define issymmetric and issubadditive, which is used in pairwise to accelerate computations.

@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2021

Codecov Report

Merging #228 (91fb7c9) into master (a43d76b) will decrease coverage by 0.08%.
The diff coverage is 98.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
- Coverage   96.82%   96.73%   -0.09%     
==========================================
  Files           8        8              
  Lines         725      736      +11     
==========================================
+ Hits          702      712      +10     
- Misses         23       24       +1     
Impacted Files Coverage Δ
src/Distances.jl 100.00% <ø> (ø)
src/bregman.jl 100.00% <ø> (ø)
src/common.jl 91.13% <ø> (+0.99%) ⬆️
src/haversine.jl 95.83% <66.66%> (-4.17%) ⬇️
src/bhattacharyya.jl 97.22% <100.00%> (+0.16%) ⬆️
src/generic.jl 98.69% <100.00%> (-0.04%) ⬇️
src/mahalanobis.jl 98.46% <100.00%> (+0.04%) ⬆️
src/metrics.jl 96.69% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a43d76b...91fb7c9. Read the comment docs.

@matthieugomez matthieugomez changed the title define IsPreMetric/IsSemiMetric/IsMetric Remove Metric <: SemiMetric <: PreMetric Sep 12, 2021
@KristofferC
Copy link
Member

This doesn't look like an improvement to me and doesn't look like standard usage of traits. Traits are typically used for small pieces of extra information that you want to dispatch on (often related to dispatching to an optimized method). This might be the type of indexing (LinearSlow or LinearFast) or if the length is known (HasLength, IsInfinite etc). But not to encode big families of types like this.

From #199:

In StringDistances, I would love to be able to define certain functions on these groups. This is not possible, however, due to the lack of multiple inheritance in Julia.

This feels like something that is more suitable for using traits for (which there should be no problem for you to do) so I don't really see that as an argument for this.

Also, remember this package is very old and has a large number of packages depending on it. Breaking changes should be kept to an absolute minimum.

@matthieugomez
Copy link
Contributor Author

matthieugomez commented Sep 12, 2021 via email

@matthieugomez
Copy link
Contributor Author

matthieugomez commented Sep 16, 2021

Types should be based on interfaces, not properties. To me, the current situation of Distances.jl feels a little bit the same as if Base defined something like

  abstract type SymmetricAbstractArray <: AbstractArray end

This would make it impossible create new array types (e.g. TriangularArray or SparseArray) that may end up being symmetric.

  • This exact problem is encountered within this package. At some point, the package tries to define a special distance type, UnionMetrics. However, because of the forced inheritance Metric <: SemiMetric <: PreMetric, the package has to do the following workaround

    abstract type UnionPreMetric <: PreMetric
    abstract type UnionSemiMetric <: SemiMetric
    abstract type UnionMetric <: Metric
    const UnionMetrics = Union{UnionPreMetric, UnionSemiMetric, UnionMetric}

    This is the only way to get around the PreMetric <: SemiMetric <: Metric forced inheritance. However, note we don't have UnionMetric <: UnionPreMetric.

  • Sometime there is no workaround to the situation. Suppose I define a parametric type that corresponds to the normalized version of a distance (say, always between 0 and 1).

    struct Normalized{T <: PreMetric} <: PreMetric
       dist::T
    end

    There is currently no way to express that Normalized(dist) remains symmetric if dist is symmetric. This is a big problem if I want to pass a normalized distance to a method that is only defined on SemiMetrics. In contrast, with this PR, one characterize the symmetry of a normalized distance in terms of the symmetry of the original distance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants