Adds options for tracking text length in text vectorizers by Jauntbox · Pull Request #195 · salesforce/TransmogrifAI

Jauntbox · 2018-12-14T21:20:41Z

Related issues
N/A

Describe the proposed solution
This PR adds options for tracking text length in the relevant text vectorizers:
SmartTextVectorizer, SmartTextMapVectorizer, TextMapHashingVectorizer, as well as the vectorize and smartVectorize shortcuts for Text, TextArea, TextMap, and TextAreaMap

Describe alternatives you've considered
N/A

Additional context
This is the second part of #187

…aps), and tests for each

…comments

…o km/text-len

…or text

…ut feature vector and tested this

…st loading since we changed the arguments to SmartTextVectorizerModel

…text-len-defaults

codecov · 2018-12-14T21:56:44Z

Codecov Report

Merging #195 into master will decrease coverage by 0.12%.
The diff coverage is 60.34%.

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
- Coverage   86.39%   86.26%   -0.13%     
==========================================
  Files         310      310              
  Lines       10019    10058      +39     
  Branches      351      548     +197     
==========================================
+ Hits         8656     8677      +21     
- Misses       1363     1381      +18

Impacted Files	Coverage Δ
.../com/salesforce/op/features/TransientFeature.scala	`63.15% <0%> (-7.44%)`	⬇️
...s/impl/feature/OPCollectionHashingVectorizer.scala	`96.47% <100%> (+0.1%)`	⬆️
...e/op/stages/impl/feature/SmartTextVectorizer.scala	`97.67% <100%> (+0.27%)`	⬆️
...sforce/op/stages/impl/feature/Transmogrifier.scala	`96.79% <100%> (+0.04%)`	⬆️
...p/stages/impl/feature/SmartTextMapVectorizer.scala	`100% <100%> (ø)`	⬆️
...n/scala/com/salesforce/op/dsl/RichMapFeature.scala	`73.23% <14.28%> (-8.3%)`	⬇️
.../scala/com/salesforce/op/dsl/RichTextFeature.scala	`84.61% <28.57%> (-5.71%)`	⬇️
...cala/com/salesforce/op/OpWorkflowModelReader.scala	`89.13% <0%> (-2.18%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 421dc9e...d6e17ea. Read the comment docs.

tovbinm · 2018-12-18T06:59:14Z

core/src/main/scala/com/salesforce/op/dsl/RichMapFeature.scala

+        case (true, true) =>
+          val textLengths = new TextMapLenEstimator[TextMap]().setInput(f +: others).getOutput()
+          val nullIndicators = new TextMapNullEstimator[TextMap]().setInput(f +: others).getOutput()
+          new VectorsCombiner().setInput(Seq(hashedFeatures, textLengths, nullIndicators): _*).getOutput()


why not .setInput(hashedFeatures, textLengths, nullIndicators)?

same elsewhere

Oh no idea why I did that - fixed

tovbinm · 2018-12-18T06:59:36Z

core/src/main/scala/com/salesforce/op/dsl/RichMapFeature.scala

      blackListKeys: Array[String] = Array.empty,
      others: Array[FeatureLike[TextMap]] = Array.empty,
      trackNulls: Boolean = TransmogrifierDefaults.TrackNulls,
+      trackTextLen: Boolean = TransmogrifierDefaults.TrackTextLen,


tovbinm · 2018-12-18T07:08:28Z

core/src/main/scala/com/salesforce/op/stages/impl/feature/SmartTextMapVectorizer.scala

+    val textLenVector = if (args.shouldTrackLen) getLenVector(keysText, rowTextTokenized) else OPVector.empty

-    categoricalVector.combine(textVector, textNullIndicatorsVector: _*)
+    categoricalVector.combine(textVector, Seq(textLenVector, textNullIndicatorsVector): _*)


categoricalVector.combine(textVector, textLenVector, textNullIndicatorsVector)

tovbinm · 2018-12-18T07:11:01Z

core/src/main/scala/com/salesforce/op/stages/impl/feature/SmartTextVectorizer.scala

    val textColumns = if (textFeatures.nonEmpty) {
-      makeVectorColumnMetadata(textFeatures, makeHashingParams()) ++ textFeatures.map(_.toColumnMetaData(isNull = true))
+      if (shouldTrackLen) {
+        makeVectorColumnMetadata(textFeatures, makeHashingParams()) ++


can this be DRYed out?

Hmmm, best I can come up with is

val textColumns = if (textFeatures.nonEmpty) { makeVectorColumnMetadata(textFeatures, makeHashingParams()) ++ (if (shouldTrackLen) textFeatures.map(_.toColumnMetaData(descriptorValue = OpVectorColumnMetadata.TextLenString)) else Array.empty[OpVectorColumnMetadata]) ++ (if (shouldTrackNulls) textFeatures.map(_.toColumnMetaData(isNull = true)) else Array.empty[OpVectorColumnMetadata]) } else Array.empty[OpVectorColumnMetadata]

which looks less readable to me...

tovbinm · 2018-12-18T07:11:31Z

core/src/main/scala/com/salesforce/op/stages/impl/feature/SmartTextVectorizer.scala

+      val textLenVector = if (args.shouldTrackLen) getLenVector(textTokens) else OPVector.empty

-      categoricalVector.combine(textVector, textNullIndicatorsVector: _*)
+      categoricalVector.combine(textVector, Seq(textLenVector, textNullIndicatorsVector): _*)


same categoricalVector.combine(textVector, textLenVector, textNullIndicatorsVector)

tovbinm · 2018-12-18T07:12:29Z

core/src/main/scala/com/salesforce/op/stages/impl/feature/Transmogrifier.scala

+/**
+ * Param that decides whether or not lengths of text are tracked during vectorization
+ */
+trait TrackTextLenParam extends Params {


once this param is added - will existing models fail to load or not?

Yes existing models will fail to load. That's why I had to re-generate the old model that we test loading with

lets indicate this in the pr description so we wont forget to include it in our release notes

tovbinm · 2018-12-18T07:14:02Z

core/src/test/scala/com/salesforce/op/stages/impl/feature/SmartTextVectorizerTest.scala

+    meta.history.keys shouldBe Set(f1.name, f2.name)
+    meta.columns.length shouldBe 12
+    meta.columns.foreach { col =>
+      if (col.index < 4) {


omg, these if/else are horrible. any better ideas?! ;)

Not at the moment. An alternative would be to do explicit comparisons on all the array indices, eg.

meta.columns(1).parentFeatureName shouldBe Seq(f1.name) meta.columns(1).grouping shouldBe None

which I think is even worse. I don't think the if/elses are that bad - they're just checking certain ranges of the feature vector. I think those explicit comparisons need to be there regardless since it's a unit test checking the output of a specific input.

tovbinm

lgtm, see comments

tovbinm · 2019-01-07T21:37:18Z

@Jauntbox are you planning to address the comments?

…rifAI into km/text-len-defaults

Jauntbox and others added 13 commits December 4, 2018 16:34

Added transformers for calculating text length (is an estimator for m…

8f6017c

…aps), and tests for each

tiny cleanup

8d5b219

Removed text cleaning ability, did some small refactoring, addressed …

bd0a48a

…comments

Merge branch 'master' into km/text-len

3a23bec

Added OpTransformerSpec and OpEstimatorSpec to tests

d3617f8

Merge branch 'km/text-len' of github.com:salesforce/TransmogrifAI int…

2343a75

…o km/text-len

Indentation

31a3206

Added TransformerSpec and EstimatorSpec to the null indcator stages f…

c45bd81

…or text

Added option to SmartTextVectorizer to track text lengths in the outp…

24ed35d

…ut feature vector and tested this

Updated vectorizer arguments and refreshed the old model we use to te…

b61e755

…st loading since we changed the arguments to SmartTextVectorizerModel

Merge branch 'master' of github.com:salesforce/TransmogrifAI into km/…

8edee13

…text-len-defaults

Added text length as an option to SmartTextVectorizer and added a test

8b56093

Added text length flag to text map vectorizers

e7a3427

Jauntbox requested review from leahmcguire and tovbinm as code owners December 14, 2018 21:20

tovbinm changed the title ~~Km/text len defaults~~ Adds options for tracking text length in text vectorizers Dec 15, 2018

tovbinm added the ready for review label Dec 15, 2018

tovbinm reviewed Dec 18, 2018

View reviewed changes

Merge branch 'master' into km/text-len-defaults

9d9e2af

Jauntbox added 2 commits January 8, 2019 10:59

Addressed comments

8c4580f

Merge branch 'km/text-len-defaults' of github.com:salesforce/Transmog…

62bc44c

…rifAI into km/text-len-defaults

tovbinm approved these changes Jan 8, 2019

View reviewed changes

make changes backwards compatible

d6e17ea

tovbinm merged commit e47b042 into master Jan 8, 2019

tovbinm deleted the km/text-len-defaults branch January 8, 2019 21:03

tovbinm mentioned this pull request Feb 7, 2019

Add length of the text as default features for text fields #187

Closed

Jauntbox mentioned this pull request Feb 8, 2019

Release 0.5.1 #222

Merged

ericwayman pushed a commit that referenced this pull request Feb 8, 2019

Adds options for tracking text length in text vectorizers (#195)

11a810e

tovbinm mentioned this pull request Jul 11, 2019

Release 3.3.3 #26

Merged

salesforce-cla bot added the cla:signed label Sep 17, 2020

Conversation

Jauntbox commented Dec 14, 2018

Uh oh!

codecov bot commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tovbinm left a comment

Choose a reason for hiding this comment

Uh oh!

tovbinm commented Jan 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Dec 14, 2018 •

edited

Loading