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.

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

LSTM error paths #3270

Merged
merged 2 commits into from
Apr 7, 2023
Merged

LSTM error paths #3270

merged 2 commits into from
Apr 7, 2023

Conversation

robertbastian
Copy link
Member

Collapsed Dictionary and LstmPayloads into one struct, ComplexPayloads. This struct is where the grapheme segmentation data can live, which is required by both code paths and introduced fallibility before. It also removes previously unhandled error cases from the core logic, so that we can log when we encounter an actual error (missing data for a language).

This also gives us a location for script properties data, which is needed by the complex code path.

@robertbastian robertbastian mentioned this pull request Apr 6, 2023
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.

Praise: Good changes. I'm merging this now to avoid conflicts.

Comment on lines +267 to +268
// Create error for logging
DataError::custom("No segmentation model for language").with_display_context(
Copy link
Member

Choose a reason for hiding this comment

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

Thought: Actually I kind-of like the idea of performing logging through the icu_provider crate. It keeps all the needed machinery in one place. We should probably add our own logging macro that we export from icu_provider.

@sffc sffc merged commit 394c359 into unicode-org:main Apr 7, 2023
@robertbastian robertbastian deleted the grapheme branch April 17, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants