Feature / Filter-low-frequency suffixes from Dense ZeroTrie #7439
Conversation
|
|
sffc
left a comment
sffc
left a comment
There was a problem hiding this comment.
Thanks for your interest in icu4x!
This MIN_DENSE_PERCENT approach seems like a decent heuristic to get us going. As usual, please make sure to add test coverage.
utils/zerotrie/src/dense.rs
Outdated
| if cursor.is_empty() { | ||
| return row_index; | ||
| } |
There was a problem hiding this comment.
Issue: This is an unnecessary change that I believe is wrong.
utils/zerotrie/src/dense.rs
Outdated
| // The row and column indexes should be in-range | ||
| debug_assert!(false); | ||
| return None; | ||
| let index = row_index |
There was a problem hiding this comment.
Nit: Using checked_add and checked_mul is beneficial, but not related to the subject of this PR. Please split those changes to a separate PR.
utils/zerotrie/src/builder/dense.rs
Outdated
| .map(|(&suffix, &count)| (suffix, count)) | ||
| .collect(); | ||
|
|
||
| // If none meet the threshold, fallback to picking top-K by frequency. |
There was a problem hiding this comment.
If none meet the threshold, then I think we should just leave the dense matrix empty.
|
Hey @sffc thanks for the review, i've tested the code (didnt push the tests) |
|
Hey @sffc , i've commited the changes, PTAL. |
3 participants
This PR addresses #7302
Implements suffix-frequency pruning to exclude rare suffixes from the dense matrix representation of ZeroAsciiDenseSparse2dTrie.