fix(kazakh): preserve protected multi-period sentinels - #92
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42949ef396
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| protect_final_period = True | ||
| elif titled_name_prefix: | ||
| protect_final_period = True | ||
| elif matched.endswith("∯"): |
There was a problem hiding this comment.
When a known two-letter initialism was already protected by the classifier because the next token matches the REGULAR I follower, this branch skips the existing split_candidate && likely_start logic below. In balanced/aggressive modes, profiles such as zh, ja, nl, it, and mr now merge I live in the U.S. I left. into one sentence, even though the same U.S. before other capital starts like How or Élodie still splits and the split-mode contract/tests expect non-English two-letter initialisms before capitals to split. Please narrow this preservation to the Kazakh case or allow the initialism split logic to override these sentinelized matches.
Useful? React with 👍 / 👎.
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Blocking feedback
- The new global sentinel short-circuit changes split-mode behavior for declared two-letter initialisms before
I ...in non-Kazakh profiles —sentencesplit/abbreviation_replacer.py#L636.
If you want me to push a fix, reply with the item number (for example: please fix 1).
| protect_final_period = True | ||
| elif titled_name_prefix: | ||
| protect_final_period = True | ||
| elif matched.endswith("∯"): |
There was a problem hiding this comment.
This matched.endswith("∯") guard runs for every language, not just Kazakh, and it bypasses the split-mode dial for any multi-period abbreviation whose final period was already protected by the classifier.
That introduces a cross-language behavior change: before this PR (d198fe2), Segmenter("it", split_mode="balanced").segment("Je vois U.S. I went.") produced ["Je vois U.S.", "I went."]; on this branch it becomes ["Je vois U.S. I went."] (same change for nl and mr).
Suggested fix: scope this preservation rule to the Kazakh regression path (for example via a Kazakh-only policy/flag or override) instead of globally short-circuiting sentinel-terminated multi-period tokens.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an opt-in abbreviation-rewrite flag, enables it for Kazakh, and adds regression coverage for Kazakh multi-period sentinels and cross-language split-mode behavior around ChangesAbbreviation boundary preservation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Segmenter
participant PeriodClassifier
participant AbbreviationReplacer
Segmenter->>PeriodClassifier: classify multi-period abbreviation
PeriodClassifier->>AbbreviationReplacer: provide protected final-period sentinel
AbbreviationReplacer->>Segmenter: preserve the classifier boundary decision
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/regression/test_kazakh_multiperiod_sentinel.py`:
- Around line 33-36: Extend
test_uppercase_dotless_multiperiod_keeps_classifier_protected_final_period to
run the same assertion for conservative, balanced, and aggressive split modes,
verifying that sentinel-terminated tokens remain unsplit regardless of the
split-mode setting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0d95f77e-3397-41e8-aec6-654d82914e2a
📒 Files selected for processing (4)
sentencesplit/abbreviation_replacer.pysentencesplit/lang/kazakh.pytests/regression/test_issues.pytests/regression/test_kazakh_multiperiod_sentinel.py
| def test_uppercase_dotless_multiperiod_keeps_classifier_protected_final_period() -> None: | ||
| seg = Segmenter("kk", split_mode="balanced") | ||
|
|
||
| assert seg.segment("Бұл Т.С.С. I went.") == ["Бұл Т.С.С. I went."] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Consider covering all split modes, not just balanced.
The PRESERVE_CLASSIFIER_PROTECTED_MULTI_PERIOD_FINAL branch is designed to short-circuit the split-mode dial for sentinel-terminated tokens, so this regression only proves the invariant for balanced. Extending to conservative/aggressive would lock in that split-mode-invariance directly.
✅ Suggested additional coverage
def test_uppercase_dotless_multiperiod_keeps_classifier_protected_final_period() -> None:
- seg = Segmenter("kk", split_mode="balanced")
-
- assert seg.segment("Бұл Т.С.С. I went.") == ["Бұл Т.С.С. I went."]
+ for mode in ("conservative", "balanced", "aggressive"):
+ seg = Segmenter("kk", split_mode=mode)
+ assert seg.segment("Бұл Т.С.С. I went.") == ["Бұл Т.С.С. I went."]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_uppercase_dotless_multiperiod_keeps_classifier_protected_final_period() -> None: | |
| seg = Segmenter("kk", split_mode="balanced") | |
| assert seg.segment("Бұл Т.С.С. I went.") == ["Бұл Т.С.С. I went."] | |
| def test_uppercase_dotless_multiperiod_keeps_classifier_protected_final_period() -> None: | |
| for mode in ("conservative", "balanced", "aggressive"): | |
| seg = Segmenter("kk", split_mode=mode) | |
| assert seg.segment("Бұл Т.С.С. I went.") == ["Бұл Т.С.С. I went."] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/regression/test_kazakh_multiperiod_sentinel.py` around lines 33 - 36,
Extend
test_uppercase_dotless_multiperiod_keeps_classifier_protected_final_period to
run the same assertion for conservative, balanced, and aggressive split modes,
verifying that sentinel-terminated tokens remain unsplit regardless of the
split-mode setting.
1 participant
Motivation
PeriodClassifieralready protected with the∯sentinel could be reclassified by the multi-period pass and have their final sentinel converted back to a literal period, causing an erroneous sentence split.Description
replace_multi_period_abbreviationsto keepprotect_final_period = Truewhen the matched token already ends with the∯sentinel, preventing the pass from undoing the classifier's decision.test_uppercase_dotless_multiperiod_keeps_classifier_protected_final_periodintests/regression/test_kazakh_multiperiod_sentinel.pythat asserts balanced-mode segmentation no longer over-splits"Бұл Т.С.С. I went.".Testing
uv run pytest tests/regression/test_kazakh_multiperiod_sentinel.py tests/lang/test_kazakh.py tests/contract/test_segmenter.py --color yesand the targeted tests passed.uv run ruff check .anduv run ruff format --check .and formatting/lint checks passed.uv run pytest --cov=sentencesplit tests/ --color yesand the test suite completed successfully (all tests passed/expected skips and xfails unchanged).Codex Task
Summary by CodeRabbit