Skip to content

deprecating jarowinkler in favor of jaro_winkler#27526

Merged
DaveCTurner merged 2 commits intoelastic:masterfrom
olcbean:distance_jaro_winkler
Nov 30, 2017
Merged

deprecating jarowinkler in favor of jaro_winkler#27526
DaveCTurner merged 2 commits intoelastic:masterfrom
olcbean:distance_jaro_winkler

Conversation

@olcbean
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

elasticmachine commented Nov 25, 2017

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
Copy Markdown
Member

dnhatn commented Nov 26, 2017

@elasticmachine please test this.

jasontedor
jasontedor requested changes Nov 26, 2017
View reviewed changes
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Member

@jasontedor jasontedor Nov 26, 2017

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
Copy Markdown
Member

@DaveCTurner DaveCTurner Nov 26, 2017

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
Copy Markdown
Member

@jasontedor jasontedor Nov 26, 2017

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
Copy Markdown
Contributor Author

olcbean commented Nov 27, 2017

I just deprecated jarowinkler and rebased.

@jasontedor
Copy link
Copy Markdown
Member

jasontedor commented Nov 27, 2017

retest this please

jasontedor
jasontedor approved these changes Nov 29, 2017
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

jasontedor commented Nov 29, 2017

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

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

@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
Copy Markdown
Member

DaveCTurner commented Nov 30, 2017

Merged and backported to 6.x in 1eb5cb1

@jasontedor
Copy link
Copy Markdown
Member

jasontedor commented Nov 30, 2017

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