Skip to content

deprecating jarowinkler in favor of jaro_winkler #27526

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 2 commits into from
Nov 30, 2017
Merged

deprecating jarowinkler in favor of jaro_winkler #27526

merged 2 commits into from
Nov 30, 2017

Conversation

olcbean
Copy link
Contributor

@olcbean olcbean commented Nov 25, 2017

Jaro-Winkler distance alg is following the naming convention used for Damerau–Levenshtein as per comment

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@dnhatn
Copy link
Member

dnhatn commented Nov 26, 2017

@elasticmachine please test this.

jasontedor
jasontedor requested changes Nov 26, 2017
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I left some questions.

core/src/main/java/org/elasticsearch/search/suggest/phrase/DirectCandidateGeneratorBuilder.java Outdated
// TODO Jaro and Winkler are 2 people - so apply same naming logic
// as damerau_levenshtein
} else if ("jarowinkler".equals(distanceVal)) {
} else if ("jaro_winkler".equals(distanceVal) || "jarowinkler".equals(distanceVal)) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to support both? Can we settle on one (with the underscore), and deprecate the other? And same for damerau_levenshtein?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for deprecation of jarowinkler.

Not sure about that score creep tho :)

Copy link
Member

Choose a reason for hiding this comment

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

Agree, the other should be separate.

@olcbean olcbean force-pushed the distance_jaro_winkler branch from 8708dcc to a7477f9 Compare November 27, 2017 14:47
@olcbean olcbean changed the title Normalizing the Jaro-Winkler distance naming within the codebase deprecating jarowinkler in favor of jaro_winkler Nov 27, 2017
@olcbean
Copy link
Contributor Author

olcbean commented Nov 27, 2017

I just deprecated jarowinkler and rebased.

@jasontedor
Copy link
Member

retest this please

jasontedor
jasontedor approved these changes Nov 29, 2017
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@jasontedor
Copy link
Member

@DaveCTurner Did you want to give this one a review too?

DaveCTurner
DaveCTurner approved these changes Nov 30, 2017
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM too.

@DaveCTurner DaveCTurner merged commit d25c967 into elastic:master Nov 30, 2017
DaveCTurner pushed a commit that referenced this pull request Nov 30, 2017
@olcbean @DaveCTurner
Deprecate jarowinkler in favor of jaro_winkler (#27526)
1eb5cb1 
Jaro and Winkler are two people, so we should use the same naming convention as for Damerau–Levenshtein.
@DaveCTurner
Copy link
Contributor

Merged and backported to 6.x in 1eb5cb1

@jasontedor
Copy link
Member

Thanks @DaveCTurner.

DaveCTurner pushed a commit that referenced this pull request Dec 11, 2017
@olcbean @DaveCTurner
Remove deprecated names for string distance algorithms (#27640)
25c606c 
#27409 deprecated the incorrectly-spelled `levenstein` in favour of `levenshtein`.
#27526 deprecated the inconsistent `jarowinkler` in favour of `jaro_winkler`.

These changes were merged into 6.2, and this change removes them entirely in 7.0.
@codebrain codebrain mentioned this pull request May 23, 2018
Closed
codebrain added a commit to elastic/elasticsearch-net that referenced this pull request May 28, 2018
@codebrain
Rename jarowinkler to jaro_winkler.
3ee7fe9 
As per: elastic/elasticsearch#27526
@codebrain codebrain mentioned this pull request May 28, 2018
Merged
codebrain pushed a commit to elastic/elasticsearch-net that referenced this pull request May 31, 2018
Rename jarowinkler to jaro_winkler. (#3263)
d097a6f 
As per: elastic/elasticsearch#27526
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasontedor jasontedor jasontedor approved these changes

@DaveCTurner DaveCTurner DaveCTurner approved these changes

Assignees
No one assigned
Labels
>deprecation :Search Relevance/Suggesters "Did you mean" and suggestions as you type v6.2.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@olcbean @elasticmachine @dnhatn @jasontedor @DaveCTurner @clintongormley @jimczi