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.

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

add favor-script variant of likely subtags algorithm #4752

Merged
merged 6 commits into from
Apr 3, 2024
Merged

add favor-script variant of likely subtags algorithm #4752

merged 6 commits into from
Apr 3, 2024

Conversation

kartva
Copy link
Member

@kartva kartva commented Mar 28, 2024 •
edited
Loading

fixes #3859

Description of changes:

  • Add LocaleExpander::minimize_favor_script.
  • Add tests for LocaleExpander::minimize and LocaleExpander::minimize_favor_script.

@kartva
Copy link
Member Author

kartva commented Mar 28, 2024

Would working on FFI bindings be better as a separate PR, or should it be part of this one?

@sffc sffc mentioned this pull request Mar 28, 2024
Merged
sffc
sffc reviewed Mar 28, 2024
View reviewed changes
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Nice work, thanks for looking at this!

While reviewing your code I realized that the minimize function could be written a lot more succinctly and I opened #4754. We should merge that PR and then you should rebase your work on top of #4754.

components/locid_transform/src/expander.rs Outdated Show resolved Hide resolved
@sffc
Copy link
Member

sffc commented Mar 28, 2024

Would working on FFI bindings be better as a separate PR, or should it be part of this one?

It saves work later if you do FFI in the same PR. If you're just adding a function, the FFI should be very straightforward.

You can do the FFI once the Rust API is finalized.

sffc added a commit that referenced this pull request Mar 30, 2024
@sffc
Simplify LocaleExpander::minimize (#4754)
b7d92c3 
I was reviewing #4752 and I realized that there's a lot of duplicate
code that can be extracted to a helper function, similar to what we do
in the `maximize` routine.

It appears to make the code slightly faster, too, which wasn't my
primary goal but is a nice side-effect.

```
likelysubtags/minimize  time:   [3.8777 µs 3.9019 µs 3.9297 µs]
                        change: [-6.9850% -6.0393% -5.0788%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  3 (3.00%) high mild
  2 (2.00%) high severe
```
@kartva kartva force-pushed the favor-script-minimize-subtags branch from d8092b1 to 3133bfd Compare March 30, 2024 20:36
sffc
sffc reviewed Mar 31, 2024
View reviewed changes
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Can you run the minimize benchmark to see if there's any performance difference across this PR?

@kartva
Copy link
Member Author

kartva commented Mar 31, 2024 

likelysubtags/minimize  time:   [3.1891 µs 3.2045 µs 3.2224 µs]
                        change: [+0.0864% +0.7153% +1.3274%] (p = 0.04 < 0.05)
                        Change within noise threshold.
Found 5 outliers among 100 measurements (5.00%)
  4 (4.00%) high mild
  1 (1.00%) high severe

@kartva
rename to minimize_impl
976124c 
make `minimize_favor_script` doc formatting consistent with `minimize`
@kartva
ffi: bindings for minimize_favor_script
870030a
sffc
sffc previously approved these changes Apr 2, 2024
View reviewed changes
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Looks good! One more request: please add this to CHANGELOG.md.

@kartva kartva requested a review from a team as a code owner April 2, 2024 21:41
sffc
sffc approved these changes Apr 3, 2024
View reviewed changes
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Nice work!

@sffc sffc merged commit c324501 into unicode-org:main Apr 3, 2024
30 checks passed
@sffc sffc mentioned this pull request Apr 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sffc sffc sffc approved these changes

@zbraniecki zbraniecki Awaiting requested review from zbraniecki

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support favor-script variant of likely subtags algorithm
2 participants
@kartva @sffc