Repository navigation
OPENNLP-1942: Test the shared deep-learning encoder and fix the token type javadoc - #1288
Conversation
|
This cannot be 1885 because it is already in the proposed M6 release candidate. It needs a new Jira. |
I'll get to this tonight |
9a7676e to
88ca5f0
Compare
rzo1
left a comment
There was a problem hiding this comment.
Thanks for the new Jira. Two things are still open from my earlier comment:
- Both commits (
c83b1f4a5,88ca5f0c0) still sayOPENNLP-1885. Please reword them toOPENNLP-1942(or we squash on merge with the PR title). - OPENNLP-1942 has
3.0.0-M6as fix version. This is not in M6, please keep only3.0.0.
Please check the commit messages and fix versions of your other open PRs that were moved to a new Jira likewise.
Missing test case: no input with a supplementary-plane character (emoji / surrogate pair) or a CJK character. Please add one for each (unknown, and CJK split).
Details inline.
Rename the test class to AbstractDLEncodeTokensTest and name the method it covers. Ids are named constants shared by the token maps and the expected arrays; the mask and segment arrays are derived from the token count. The concurrency test is dropped, the duplicate encoding case in CreateTokenizerTest is removed, and the RoBERTa wordpiece case is marked as covering wordpiece with RoBERTa special tokens, not byte-level BPE. New cases: a supplementary-plane symbol as a known token, as an unknown token and inside a word; CJK runs split into single characters under both case settings; known punctuation and a punctuation run; special token text in the input; a removed control character and a removed zero width space; and the word length limit at 100 and 101 code points. The Tokens javadoc now states the attention mask is all 1.
88ca5f0 to
3d5943c
Compare
|
Thanks for the review. Pushed 3d5943c, rebased on current main:
On the other PRs: #1236 is titled OPENNLP-547 with OPENNLP-1888 commits, and #1238 has no ticket in its commit subjects. I will fix both when I next touch those branches. |
Follow-up to #1165 on OPENNLP-1942, based on main and independent of the rest.
#1165 routed
DocumentCategorizerDL,NameFinderDLandSentenceVectorsDLthrough oneAbstractDL.encodeTokens. The per-component tests that had checked the token and id arrays were removed along with the duplicated code they covered, leaving one encoding case inCreateTokenizerTest. This adds a test class for that method, plus two javadoc fixes found while writing it.AbstractDLEncodeTokensTestchecks the tokens, ids, attention mask and segment ids ofAbstractDL.encodeTokensin 36 cases without a model file:StringBuilderinput[CLS]as input text, which is split as text and not mapped to the special idIds are named constants shared between the token maps and the expectations, and include 0 and
Integer.MAX_VALUEso a sign or ordering slip fails. The mask and segment arrays are derived from the token count. A further test checks that repeated calls return independent arrays.ModelFreeDLis the model-free subclass both test classes share;CreateTokenizerTestcovers creation only.Javadoc fixes:
Tokens.maskandTokens.typesnow say what the encoder writes: the attention mask is all 1, the segment ids are all 0 for a single segment. The old text said types were "typically all 1".WordpieceEncoderlinksSpecialCasing.txtwhere it mentions the Unicode full case mapping, matching theWordBreakProperty.txtreference already there.No production behavior changes.
Verification, offline, with checkstyle:
AbstractDLEncodeTokensTest38 tests,CreateTokenizerTest11 tests, no failures.