rzo1 commented on PR #1106:
URL: https://github.com/apache/opennlp/pull/1106#issuecomment-4883252482
Hi @krickert. I didn't have time to deeply review yet, so here you get some
Fable 5 output. Take it with a grain of salt — I will read it next week myself
(human in the loop).
**The good news first:** the model cross-checked the docs against the actual
code on the branch and found the technical content fully accurate — all
constructors/signatures in the samples exist as shown, all 12 `xref` targets
resolve, XML is well-formed with the header matching the post-OPENNLP-1854 DTD
convention, `defaultChain()` order matches the prose, and every expected-output
comment (`"cafe"`, the span arithmetic, `3.0 : NUMERIC`, …) is pinned by an
existing unit test. Everything below is clarity/duplication/structure, no
factual errors.
**Findings (most severe first):**
1. **Duplication with drift** (`namefinder.xml`, `doccat.xml`,
`normalizer.xml`): the InferenceOptions folding + White_Space chunking story is
told near-verbatim in three chapters, and the copies already disagree in
wording ("folds each Unicode whitespace *character*" vs. "maps each Unicode
whitespace *code point*" vs. "one code point to one space"). Suggestion:
component chapters keep one sentence + the existing xref to
`tools.normalizer.dl` as the single canonical explanation.
2. **`tokenizer.xml` intro**: "The DL components apply that machinery
automatically for document chunking" reads as if DL normalization is automatic,
contradicting the "off by default" statements elsewhere — only *chunking* is
automatic. The new paragraph also splits the intro sentence from the `<screen>`
example it introduces. Suggested rewording: "The DL components perform
Unicode-aware chunking automatically; the folding transforms remain opt-in."
3. **`doccat.xml`**: the supplementary-plane dash / UTF-16 offset caveat is
spelled out in full, but `DocumentCategorizerDL` exposes no span/offset API, so
readers get surrogate-pair arithmetic they can never observe. One clause + xref
would do.
4. **`namefinder.xml`**: the second code sample repeats ~14 lines of the
first (model, vocab, full 9-entry `ids2Labels` map, sentence detector) to add
three `InferenceOptions` lines — and the copies already drift (different tokens
arrays, different comments). Could be trimmed to just the options + constructor
call.
5. **`normalizer.xml`**: "rungs" is used as terminology three times but
never introduced, while the same file also says "stage" for the same concept.
Pick one (plain "stage" reads fine).
6. **`namefinder.xml`/`doccat.xml`**: the offset-preservation sentence
("Whitespace folding is one code point to one character …, and so is dash
folding for Basic Multilingual Plane dashes; a supplementary-plane dash shrinks
from two UTF-16 units to one …") is a garden path with mixed units and
unglossed jargon. Splitting into three short sentences (whitespace always safe
→ BMP dashes safe → rare non-BMP dashes shift, `findInOriginal` compensates)
would fix it.
7. **`normalizer.xml` offset-aware section**: two very dense sentences — the
`buildAligned()` description with three nested "returns", and a ~63-word
sentence ending "an offset that does not hold". Both accurate, both worth
splitting.
8. **`normalizer.xml` intro**: the "three principles" describe
implementation strategy ("cursor-based, no regular expressions", "O(1)",
"ReDoS"), and this internals-talk recurs ~8 more times across three chapters.
The user-relevant contract is "Unicode-correct per the UCD, offset-preserving"
— the no-regex/performance story could be stated once (or left to Javadoc) so
the manual doesn't freeze internals as promises.
**Minor:** "lists the 25 characters carrying the White_Space property" will
silently go stale on a UCD upgrade; the Catalan middle dot exemption in the
bullet-normalizer table would benefit from the one-word reason (U+00B7 occurs
inside Catalan words); "adding an unrequested dimension costs one transform"
reads as a contradiction until you realize "adding" means "querying at runtime".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
