rzo1 commented on PR #1155:
URL: https://github.com/apache/opennlp/pull/1155#issuecomment-4938604597
Hi @krickert - as mentioned on Slack I currently dont have the time for a
manual review but I just let Fable do a comprehensive review on this PR. Here
is the result:
-
`opennlp-extensions/opennlp-wordnet/src/main/java/opennlp/wordnet/WnLmfReader.java`:
SynsetRelation elements with relType="other" are rejected with "Unknown
relation type other", although the class javadoc documents that "other"
relations are skipped. The skip filter is only applied to SenseRelations in
build(); SynsetRelations go through parseRelation unconditionally, so a WN-LMF
document that uses relType="other" on a SynsetRelation (permitted by the DTD,
emitted by several OMW-family wordnets) fails to load. Tests only cover the
SenseRelation case.
-
`opennlp-extensions/opennlp-wordnet/src/main/java/opennlp/wordnet/MorphyLemmatizer.java`:
posFromTag's first-character fallback is broader than its javadoc claims. The
single-letter WordNet codes "a" and "s" are matched as prefixes, so
multi-letter tags like UD AUX, ADP, SCONJ or Penn SYM are misclassified as
ADJECTIVE instead of yielding the documented unknown-word result (e.g. AUX
"was" is looked up as an adjective and returns "O"). The "a"/"s" cases should
require tag.length() == 1, and testPosFromTagMapping should exercise such tags.
- `pom.xml`: The new opennlp-wordnet artifact is not registered in the root
pom's dependencyManagement and not added as a dependency of opennlp-distr,
unlike the other extension modules (opennlp-morfologik, opennlp-spellcheck,
opennlp-uima). As-is it will not ship in the binary distribution or aggregated
apidocs, and reactor modules cannot reference it without an explicit version.
-
`opennlp-extensions/opennlp-wordnet/src/main/java/opennlp/wordnet/WndbReader.java`:
The new public loading entry points (WndbReader.read, WnLmfReader.read,
MorphyExceptions.load) signal I/O failures as UncheckedIOException and
malformed data as IllegalArgumentException, diverging from the established
convention of checked IOException / InvalidFormatException used by comparable
loaders (DictionaryLemmatizer, model loaders). Once released this contract
cannot be tightened without breaking source compatibility, so it should be
decided deliberately before the first release.
-
`opennlp-extensions/opennlp-wordnet/src/main/java/opennlp/wordnet/WnLmfReader.java`:
The "similar"-on-verb-synset to VERB_GROUP mapping has zero test coverage in
either reader; the WN-LMF fixture only carries "similar" on adjective synsets
and the WNDB fixture has no "$" pointer, so a regression in the only
POS-dependent relType mapping would pass the whole suite.
-
`opennlp-extensions/opennlp-wordnet/src/main/java/opennlp/wordnet/WnLmfReader.java`:
Duplicate Sense ids and duplicate LexicalEntry ids are silently overwritten
(plain map.put) while duplicate Synset ids fail loud, so a malformed document
can have sense relations lifted onto the wrong synset without any error.
- `opennlp-api/src/main/java/opennlp/tools/wordnet/WordNetPos.java`: The
name uses mixed-case "Pos" while the rest of the codebase uppercases the
abbreviation (POSTagger, POSModel, POSSample); this public type cannot be
renamed after release, so consider WordNetPOS or a spelled-out name.
- `opennlp-api/src/main/java/opennlp/tools/wordnet/Synset.java`: Synset is a
public record whose canonical five-component constructor becomes frozen API;
since the seam explicitly anticipates additive extension, adding a component
later requires permanently keeping an overload of the old constructor. Worth
confirming the v1 component set (per-sense data, lexicographer file) before
release.
-
`opennlp-extensions/opennlp-wordnet/src/main/java/opennlp/wordnet/WndbReader.java`:
Relation-target synset id strings are minted fresh per pointer and never
canonicalized against the synset table (~660K pointers in Princeton 3.0),
retaining tens of MB of duplicate short Strings in a lexicon designed to be
held in memory long-term. Interning against the canonical RawSynset.id during
resolve() would collapse them; the same applies to WnLmfReader.resolveRelations.
-
`opennlp-extensions/opennlp-wordnet/src/main/java/opennlp/wordnet/InMemoryWordNetLexicon.java`:
The constructor's referential-integrity validation (key/id mismatch, dangling
relation target, sense-order entry referencing unknown synset) has no direct
test; a future third reader would rely on these checks and a regression would
go unnoticed.
-
`opennlp-extensions/opennlp-wordnet/src/main/java/opennlp/wordnet/WndbReader.java`:
The RawPointer record's lineNumber component is dead code; it is populated at
every parse but never read since resolve() reports errors with the owning
synset's location. Either drop it or use it for pointer-precise error messages.
-
`opennlp-extensions/opennlp-wordnet/src/main/java/opennlp/wordnet/MorphyExceptions.java`:
The lemma-folding logic (replace('_',' ').toLowerCase(Locale.ROOT)) is
duplicated verbatim in InMemoryWordNetLexicon.LemmaKey.of, and splitOnSpaces is
duplicated in WnLmfReader.Parser. The fold agreement is load-bearing for
MorphyLemmatizer, but nothing (shared helper or cross-referencing test)
prevents the copies from drifting apart.
-
`opennlp-extensions/opennlp-wordnet/src/test/java/opennlp/wordnet/WnLmfReaderTest.java`:
Several documented fail-loud paths are untested: duplicate synset id,
synset/member POS mismatch, SenseRelation targeting an undeclared sense, and
the structural-misplacement errors (Lemma outside LexicalEntry, Sense before
Lemma, SenseRelation outside Sense, SynsetRelation outside Synset).
Human review will follow.
--
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]
