Skip to content

[Merged by Bors] - feat: Estimator typeclass and implementation for Levenshtein distance#6119

Closed
kim-em wants to merge 69 commits intomasterfrom
edit_distance_estimator_new
Closed

[Merged by Bors] - feat: Estimator typeclass and implementation for Levenshtein distance#6119
kim-em wants to merge 69 commits intomasterfrom
edit_distance_estimator_new

Conversation

@kim-em
Copy link
Copy Markdown
Contributor

@kim-em kim-em commented Jul 25, 2023

@kim-em kim-em added blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) awaiting-review awaiting-CI This PR does not pass CI yet. This label is automatically removed once it does. labels Jul 25, 2023
@ghost ghost added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Aug 16, 2023
@ghost ghost removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Aug 16, 2023
Comment on lines +82 to +84
rw [e.distances_eq] at eq
simp only [List.getElem_eq_get] at eq
rw [split] at eq
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's happening here? Can't you reduce this to a single simp only or at least a simp_rw?

Copy link
Copy Markdown
Contributor Author

@kim-em kim-em Aug 29, 2023

Choose a reason for hiding this comment

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

Unfortunately not: changing the first rw to a simp_rw causes the last rw to error with a "motive not type correct". simp doesn't make any progress when used in place of the last rw.

Ugly, hey? I'm inclined to say this is just unfixable DTT unpleasantness.

Comment on lines +101 to +103
rw [e.distances_eq] at eq
simp only [List.getElem_eq_get] at eq
rw [split] at eq
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same remark here. I really feels like the simp set isn't good enough.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As above.

@ghost ghost removed the blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) label Aug 29, 2023
@PatrickMassot
Copy link
Copy Markdown
Member

bors merge

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels Aug 29, 2023
bors bot pushed a commit that referenced this pull request Aug 29, 2023
…#6119)

Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Co-authored-by: Thomas Browning <tb65536@uw.edu>
Co-authored-by: Chris Hughes <chrishughes24@gmail.com>
Co-authored-by: Oliver Nash <github@olivernash.org>
Co-authored-by: sgouezel <sebastien.gouezel@univ-rennes1.fr>
Co-authored-by: Alex J Best <alex.j.best@gmail.com>
Co-authored-by: Xavier Roblot <46200072+xroblot@users.noreply.github.com>
Co-authored-by: michaellee94 <michael_lee1@brown.edu>
@bors
Copy link
Copy Markdown

bors bot commented Aug 29, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title feat: Estimator typeclass and implementation for Levenshtein distance [Merged by Bors] - feat: Estimator typeclass and implementation for Levenshtein distance Aug 29, 2023
@bors bors bot closed this Aug 29, 2023
@bors bors bot deleted the edit_distance_estimator_new branch August 29, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge This PR has been sent to bors. t-combinatorics Combinatorics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants