Skip to content

Add thai keyboard distance #513

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 14 commits into from
Jan 4, 2021
Merged

Add thai keyboard distance #513

merged 14 commits into from
Jan 4, 2021

Conversation

ppirch
Copy link
Contributor

@ppirch ppirch commented Dec 31, 2020

What does this changes

Add function for calculate euclidean distance between two Thai characters on QWERTY Layout Keyboard

@pep8speaks
Copy link

pep8speaks commented Dec 31, 2020 •
edited
Loading

Hello @ppirch! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-01-04 02:18:48 UTC

@coveralls
Copy link

coveralls commented Dec 31, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.009%) to 95.836% when pulling 94649af on ppirch:add-thai-keyboard-distance into c2e65bc on PyThaiNLP:dev.

@bact
Copy link
Member

bact commented Dec 31, 2020

Thanks for great contribution.
Do you mind to change the name of the layout from QWERTY to "Kedmanee" (or TIS 820 / TIS 820-2531 / TIS 820-2538, if that matter) ?
https://en.wikipedia.org/wiki/Thai_Kedmanee_keyboard_layout

@bact bact added the enhancement enhance functionalities label Dec 31, 2020
@bact bact added this to the 2.3 milestone Dec 31, 2020
@ppirch
Copy link
Contributor Author

ppirch commented Dec 31, 2020

Thanks for great contribution.
Do you mind to change the name of the layout from QWERTY to "Kedmanee" (or TIS 820 / TIS 820-2531 / TIS 820-2538, if that matter) ?
https://en.wikipedia.org/wiki/Thai_Kedmanee_keyboard_layout

I agree to that suggestion.
I changed the name from QWERTY to TIS 820-2538.

@bact bact requested a review from wannaphong January 1, 2021 06:32
wannaphong
wannaphong reviewed Jan 1, 2021
View reviewed changes
@wannaphong wannaphong requested a review from bact January 2, 2021 06:01
@wannaphong
Copy link
Member

wannaphong commented Jan 2, 2021 •
edited
Loading

I have checked the algorithm. It's passed. get_char_coord output is x,y. The formula is formula

bact added 2 commits January 3, 2021 15:18
@bact
Fix keyboard layout array
51cde15 
- Fix: According to the current distance algorithm, the keyboard layout should be a 2D array (in this case, 4 rows x N columns)
  - Try "๘"-"๙" and "๙"-"๐", they should have different distance values.
- Fix: The proposed layout is actually a modified TIS 820-2531 (not 2538 one, see docstring)
@bact
Add keyboard distance test
1221564 
Thai digit zero (๐) shouldn't be "next to" Thai digit nine (๙).
Their distance should be different from the distance between, say, Thai digit eight (๘) and Thai digit nine (๙).
bact
bact approved these changes Jan 3, 2021
View reviewed changes
Copy link
Member

@bact bact left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@ppirch I have committed my proposed code, please kindly check if I understand your algorithm correctly.

I basically changed the keyboard layout array from 1D to 2D.
Since the distance between Thai digit eight and nine should be different from the distance between Thai digit nine and zero.
These two pairs are both next to each other in 1D array, but it's not the case for 2D keyboard layout.

A new test case also added to illustrated this.

I also changed the constant names to refer to a modified TIS 820 2531 instead. Since that's more reflected the content inside the array.

@bact bact mentioned this pull request Jan 4, 2021
Closed
@bact bact merged commit 3103449 into PyThaiNLP:dev Jan 4, 2021
@bact bact mentioned this pull request Jan 4, 2021
Merged
@bact
Copy link
Member

bact commented Jan 4, 2021 •
edited
Loading

Sorry, I made a mistake in reverting the merge. Reverting the revert, and this PR is safely restored.

Thanks @ppirch again for the code.

@wannaphong wannaphong mentioned this pull request Apr 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bact bact bact approved these changes

@wannaphong wannaphong wannaphong approved these changes

Assignees

No one assigned

Labels

enhancement enhance functionalities

Projects

None yet

Milestone

2.3

Development

Successfully merging this pull request may close these issues.

5 participants

@ppirch @pep8speaks @coveralls @bact @wannaphong