Adjust bin values for text features in RFF #99

sxd929 · 2018-08-28T18:28:46Z

Related issues
Not enough hashing space for RFF of text may reduce the js distance (weaken the signal)

Describe the proposed solution

Add sum and total count to summary
Allow user provided method to calculate the bin number based on data summary as parameter to calculate the distribution

Describe alternatives you've considered
Alternative method like Jaccard similarity, word and sentence embedding might be considered

Additional context
N/A

codecov · 2018-08-28T19:01:06Z

Codecov Report

Merging #99 into master will decrease coverage by <.01%.
The diff coverage is 89.18%.

@@            Coverage Diff             @@
##           master      #99      +/-   ##
==========================================
- Coverage   86.08%   86.07%   -0.01%     
==========================================
  Files         297      297              
  Lines        9659     9660       +1     
  Branches      336      429      +93     
==========================================
  Hits         8315     8315              
- Misses       1344     1345       +1

Impacted Files	Coverage Δ
.../src/main/scala/com/salesforce/op/OpWorkflow.scala	`87.5% <ø> (ø)`	⬆️
...main/scala/com/salesforce/op/filters/Summary.scala	`100% <100%> (ø)`	⬆️
...a/com/salesforce/op/filters/RawFeatureFilter.scala	`92.23% <75%> (-1.04%)`	⬇️
...a/com/salesforce/op/filters/PreparedFeatures.scala	`78.37% <75%> (-1.11%)`	⬇️
...om/salesforce/op/filters/FeatureDistribution.scala	`98% <95%> (+0.12%)`	⬆️

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 83baade...dfc1a40. Read the comment docs.

tovbinm · 2018-08-28T19:06:47Z

core/src/main/scala/com/salesforce/op/filters/FeatureDistribution.scala

+        val maxBins = MaxBins
+        val numBins = math.min(math.max(bins, sum.max/AvgBinValue), maxBins).toInt
+
+        val hasher: HashingTF = new HashingTF(numFeatures = numBins)


do we have to create hasher every time or perhaps we can create it once?

the hashing dimension can be different for different features, the minimum number would be bins
but we can have a shared one with numFeatures = bins, and use that for every case if there is not too many tokens; OR we can create a couple shared hashers with different scales, and choose one based on the scale of token numbers
What do you think? I was assuming creating a hasher for every feature will not be very resource consuming

let see if we can reuse the hashing function without creating HashingTF everytime.

and I think we can. See HashingTF.transform and HashingTF object

tovbinm · 2018-08-28T19:07:18Z

core/src/main/scala/com/salesforce/op/filters/FeatureDistribution.scala

+      case Left(seq) => {
+        val minBins = bins
+        val maxBins = MaxBins
+        val numBins = math.min(math.max(bins, sum.max/AvgBinValue), maxBins).toInt


sum.max / AvgBinValue

instead of .toInt perhaps specify the explicit rounding instead.

fixed! thanks!!

leahmcguire · 2018-08-28T22:25:13Z

core/src/main/scala/com/salesforce/op/filters/FeatureDistribution.scala

+      case Left(seq) => {
+        val minBins = bins
+        val maxBins = MaxBins
+        val numBins = math.min(math.max(bins, sum.max / AvgBinValue), maxBins).floor


may be we should make this more flexible

instead of defining this allow users to pass in a function for text bins eg:
val textBins: (min: Int, max: Int, sum: Long, count: Long) => Int
to the contructor

and add sum to the Summary info collected

tovbinm · 2018-08-30T00:46:20Z

core/src/main/scala/com/salesforce/op/filters/FeatureDistribution.scala

@@ -142,6 +143,13 @@ case class FeatureDistribution
 private[op] object FeatureDistribution {

  val MaxBins = 100000
+  val AvgBinValue = 5000
+  val MaxTokenLowerLimit = 10
+  val getBins = (sum: Summary, bins: Int) => {


let's add move this method to Summary class., ie.

private[op] case class Summary(min: Double, max: Double) { // TODO: docs def bins(bins: Int): Int = { if (sum.max < MaxTokenLowerLimit) bins else math.min(math.max(bins, sum.sum / AvgBinValue), MaxBins).toInt } }

…rce/TransmogrifAI into xs/adjustBinValueForText

tovbinm

Lgtm, please replace Todo with TODO @sxd929

Shall we merge? @leahmcguire

leahmcguire · 2018-08-31T15:49:30Z

core/src/main/scala/com/salesforce/op/filters/Summary.scala

+   * @param bins default bins of RFF
+   * @return
+   */
+  def getBinsText(bins: Int): Int = bins


The user should be able to pass the formula for how to compute the text feature bin size into RawFeatureFilter as a parameter. We should provide a default which for now can be no formula but later can be updated

Jauntbox · 2018-08-31T20:48:21Z

core/src/test/scala/com/salesforce/op/filters/FeatureDistributionTest.scala

-      Map("A" -> Summary(-1.6, 10.6), "B" -> Summary(0.0, 3.0)),
-      Map("B" -> Summary(0.0, 0.0)))
+    val summaries = Array(
+      Map("A" -> Summary(0.0, 2.0, 100.0, 10), "B" -> Summary(0.0, 5.0, 10.0, 10)),


Why did the max change for just the A key here?

This is hardcoded, I changed it since a sample of "A" is Seq("male", "female"), number of hashes in the text is already 2, so the max should not be smaller than 2

Jauntbox · 2018-08-31T20:50:11Z

core/src/test/scala/com/salesforce/op/filters/FeatureDistributionTest.scala

@@ -107,15 +119,15 @@ class FeatureDistributionTest extends FlatSpec with PassengerSparkFixtureTest wi
      else d.distribution.length shouldBe 2
    }
    distribs(0).nulls shouldBe 0
-    distribs(0).summaryInfo should contain theSameElementsAs Array(0.0, 1.0)
+    distribs(0).summaryInfo should contain theSameElementsAs Array(0.0, 2.0, 100.0, 10.0)


Same here, why did the max change?

this one reads the info from hardcoded 'summaries' above

leahmcguire · 2018-08-31T21:25:48Z

core/src/main/scala/com/salesforce/op/filters/FeatureDistribution.scala

+    textBinsFormula: (Summary, Int) => Int
+  ): (Array[Double], Array[Double]) = values match {
+    case Left(seq) =>
+      val numBins = textBinsFormula(summary, bins)


why would the formula need the bins?

Right now we set is as def textBinsFormula(s: Summary, bins: Int) = bins.
I can image default bins can be returned in some cases.

what would you recommend then? @leahmcguire

bins is the default number (e.g. 100) there might be cases that we will just want to fall back to the defaults

leahmcguire · 2018-08-31T21:26:02Z

core/src/main/scala/com/salesforce/op/OpWorkflow.scala

@@ -514,7 +517,8 @@ class OpWorkflow(val uid: String = UID[OpWorkflow]) extends OpWorkflowCore {
    maxJSDivergence: Double = 0.90,
    maxCorrelation: Double = 0.95,
    correlationType: CorrelationType = CorrelationType.Pearson,
-    protectedFeatures: Array[OPFeature] = Array.empty
+    protectedFeatures: Array[OPFeature] = Array.empty,
+    textBinsFormula: (Summary, Int) => Int = RawFeatureFilter.textBinsFormula


why the int?

it's the bins.

Yeah but that is not what we want the formula to look like long term

so what is your suggestion on the inputs / outputs then?

leahmcguire

Please just add to comments what the bins are

salesforce-cla · 2020-10-28T16:36:01Z

Thanks for the contribution! It looks like @sxd929 is an internal user so signing the CLA is not required. However, we need to confirm this.

sxd929 added 2 commits August 28, 2018 11:02

adjust bin value based on number of token

da5ba89

cleanup

1d90e8f

sxd929 requested review from Jauntbox and leahmcguire August 28, 2018 18:28

sxd929 requested a review from tovbinm as a code owner August 28, 2018 18:28

sxd929 added the ready for review label Aug 28, 2018

tovbinm changed the title ~~Xs/adjust bin value for text~~ Adjust bin values for text features in RFF Aug 28, 2018

tovbinm reviewed Aug 28, 2018

View reviewed changes

sxd929 added 2 commits August 28, 2018 12:19

address comments

3b6c45f

merge

eea1cf2

leahmcguire reviewed Aug 28, 2018

View reviewed changes

sxd929 added work in progress DO NOT MERGE and removed ready for review labels Aug 28, 2018

sxd929 and others added 4 commits August 29, 2018 10:45

add sum and count to summary

b958887

change formula

a54131b

add todo

dae829d

Merge branch 'master' into xs/adjustBinValueForText

d6ad74f

tovbinm reviewed Aug 30, 2018

View reviewed changes

sxd929 added 2 commits August 30, 2018 13:52

use bins as default

d9dee94

Merge branch 'xs/adjustBinValueForText' of https://github.com/salesfo…

1bb28c6

…rce/TransmogrifAI into xs/adjustBinValueForText

sxd929 added ready for review and removed DO NOT MERGE work in progress labels Aug 30, 2018

sxd929 and others added 4 commits August 30, 2018 13:54

Merge branch 'master' into xs/adjustBinValueForText

441ab06

Merge branch 'master' into xs/adjustBinValueForText

546fcfc

Merge branch 'master' into xs/adjustBinValueForText

13cb288

address comments

c47e9bc

sxd929 added 4 commits August 30, 2018 17:02

cleanup

e8d616b

Merge branch 'xs/adjustBinValueForText' of https://github.com/salesfo…

e133bb4

…rce/TransmogrifAI into xs/adjustBinValueForText

cleanup

20a9c40

Merge branch 'master' into xs/adjustBinValueForText

e39dad6

tovbinm approved these changes Aug 31, 2018

View reviewed changes

Merge branch 'master' into xs/adjustBinValueForText

1c6d359

leahmcguire requested changes Aug 31, 2018

View reviewed changes

tovbinm added 2 commits August 31, 2018 11:14

Added textBinsFormula to RFF

5e2857c

make scalastyle happy

a2ffd3a

Jauntbox reviewed Aug 31, 2018

View reviewed changes

leahmcguire reviewed Aug 31, 2018

View reviewed changes

tovbinm added 4 commits August 31, 2018 14:31

Merge branch 'master' into xs/adjustBinValueForText

3c35a13

Merge branch 'master' into xs/adjustBinValueForText

2f4f77a

Merge branch 'master' into xs/adjustBinValueForText

6187e55

Merge branch 'master' into xs/adjustBinValueForText

7ff99fe

leahmcguire approved these changes Sep 4, 2018

View reviewed changes

tovbinm added 2 commits September 4, 2018 10:56

update docs

bdc0608

Added docs

dfc1a40

tovbinm merged commit 2d7c275 into master Sep 4, 2018

tovbinm deleted the xs/adjustBinValueForText branch September 4, 2018 18:26

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

Adjust bin values for text features in RFF (#99)

ec688b3

salesforce-cla bot added the cla:signed label Jul 8, 2020

salesforce-cla bot added cla:missing and removed cla:signed labels Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust bin values for text features in RFF #99

Adjust bin values for text features in RFF #99

sxd929 commented Aug 28, 2018 •

edited by tovbinm

codecov bot commented Aug 28, 2018 •

edited

tovbinm Aug 28, 2018

sxd929 Aug 28, 2018

tovbinm Aug 28, 2018

tovbinm Aug 28, 2018

tovbinm Aug 28, 2018

tovbinm Aug 28, 2018 •

edited

sxd929 Aug 28, 2018

leahmcguire Aug 28, 2018

tovbinm Aug 30, 2018 •

edited

sxd929 Aug 31, 2018

tovbinm left a comment

leahmcguire Aug 31, 2018

tovbinm Aug 31, 2018

Jauntbox Aug 31, 2018

sxd929 Aug 31, 2018 •

edited

Jauntbox Aug 31, 2018

sxd929 Aug 31, 2018

leahmcguire Aug 31, 2018

tovbinm Aug 31, 2018

tovbinm Aug 31, 2018

sxd929 Aug 31, 2018

leahmcguire Aug 31, 2018

tovbinm Aug 31, 2018

leahmcguire Aug 31, 2018

tovbinm Aug 31, 2018

leahmcguire left a comment

salesforce-cla bot commented Oct 28, 2020

Adjust bin values for text features in RFF #99

Adjust bin values for text features in RFF #99

Conversation

sxd929 commented Aug 28, 2018 • edited by tovbinm

codecov bot commented Aug 28, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tovbinm Aug 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tovbinm Aug 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tovbinm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sxd929 Aug 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leahmcguire left a comment

Choose a reason for hiding this comment

salesforce-cla bot commented Oct 28, 2020

sxd929 commented Aug 28, 2018 •

edited by tovbinm

codecov bot commented Aug 28, 2018 •

edited

tovbinm Aug 28, 2018 •

edited

tovbinm Aug 30, 2018 •

edited

sxd929 Aug 31, 2018 •

edited