This repository has been archived by the owner. It is now read-only.

Adding TextToTermIndexSequenceTransform #408

Merged
merged 4 commits into from Aug 29, 2017

Conversation

Projects
None yet
2 participants
@turambar
Copy link
Contributor

turambar commented Aug 28, 2017

What changes were proposed in this pull request?

Adding a TextToTermIndexSequenceTransform (similar to TextToCharacterIndexTransform but with terms).

How was this patch tested?

Added a unit test to TestTransforms.

@AlexDBlack
Copy link
Member

AlexDBlack left a comment

Only the test has been pushed up, not the actual class.

@AlexDBlack
Copy link
Member

AlexDBlack left a comment

Minor changes required, otherwise LGTM.

* @author Dave Kale
*/
@Data
@EqualsAndHashCode(callSuper = true, exclude = {""})

This comment has been minimized.

@AlexDBlack

AlexDBlack Aug 28, 2017

Member

probably exclude={"writableMap"}

*/
@Data
@EqualsAndHashCode(callSuper = true, exclude = {""})
@JsonIgnoreProperties({"wordIndexMap", "writableMap"})

This comment has been minimized.

@AlexDBlack

AlexDBlack Aug 28, 2017

Member

Remove "wordIndexMap" otherwise your transform can't be (de)serialized from/to JSON (your vocab won't be present).

@turambar

This comment has been minimized.

Copy link
Contributor

turambar commented Aug 29, 2017

@AlexDBlack addressed feedback on JSON annotations, and added a unit test to test serialization. However, the unit test fails because it looks like the call to equals is not properly ignoring the writableMap field as specified in the EqualsAndHashCode annotation. Any thoughts would be much appreciated!

Fix Lombok and Jackson annotations so comparisons work for
BaseSequenceTransform and subclasses.
@turambar

This comment has been minimized.

Copy link
Contributor

turambar commented Aug 29, 2017

@AlexDBlack: I fixed the annotations on BaseSequenceTransform and the new TextToTermIndexSequenceTransform -- unit test now passes.

@turambar turambar self-assigned this Aug 29, 2017

@AlexDBlack
Copy link
Member

AlexDBlack left a comment

LGTM

@turambar turambar merged commit 24e84c3 into master Aug 29, 2017

@turambar turambar deleted the dck_textSequence branch Aug 29, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.