Remove duplicate features using sanity checker feature to feature correlations #476

leahmcguire · 2020-05-11T16:16:37Z

Related issues
Refer to issue(s) addressed in this pull request from Issues page.

Describe the proposed solution
When features are exact duplicates of each other we would like to remove one before modeling

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context about the changes here.

codecov · 2020-05-11T16:36:04Z

Codecov Report

Merging #476 into master will increase coverage by 0.03%.
The diff coverage is 95.16%.

@@            Coverage Diff             @@
##           master     #476      +/-   ##
==========================================
+ Coverage   87.00%   87.03%   +0.03%     
==========================================
  Files         345      345              
  Lines       11643    11671      +28     
  Branches      376      614     +238     
==========================================
+ Hits        10130    10158      +28     
  Misses       1513     1513

Impacted Files	Coverage Δ
...ala/com/salesforce/op/dsl/RichNumericFeature.scala	`100.00% <ø> (ø)`
...rce/op/stages/impl/preparators/SanityChecker.scala	`91.25% <90.90%> (-0.24%)`	⬇️
...tages/impl/preparators/SanityCheckerMetadata.scala	`89.72% <95.12%> (+0.74%)`	⬆️
...c/main/scala/com/salesforce/op/ModelInsights.scala	`93.04% <100.00%> (-0.07%)`	⬇️
...s/impl/preparators/DerivedFeatureFilterUtils.scala	`93.08% <100.00%> (+0.31%)`	⬆️
...es/src/main/scala/com/salesforce/op/OpParams.scala	`89.79% <0.00%> (+4.08%)`	⬆️

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 24cdbc4...ac1c3a7. Read the comment docs.

nicodv

Nice addition. Left several minor comments.

nicodv · 2020-05-13T18:32:37Z

core/src/test/scala/com/salesforce/op/stages/impl/preparators/SanityCheckerTest.scala

@@ -640,11 +671,13 @@ class SanityCheckerTest extends OpEstimatorSpec[OPVector, BinaryModel[RealNN, OP

 val metadata: Metadata = getMetadata(outputColName, transformedData)
 val summary = SanityCheckerSummary.fromMetadata(metadata.getSummaryMetadata())
-
+println(summary.correlations)


nicodv · 2020-05-13T18:34:50Z

core/src/test/scala/com/salesforce/op/stages/impl/preparators/BadFeatureZooTest.scala

@@ -98,6 +99,7 @@ class BadFeatureZooTest extends FlatSpec with TestSparkContext with Logging {

 // Check that all the PickList features are dropped and they are the only ones dropped
 // (9 choices + "other" + empty = 11 total dropped columns)
+ println(retrieved.dropped)


core/src/main/scala/com/salesforce/op/stages/impl/preparators/DerivedFeatureFilterUtils.scala

nicodv · 2020-05-13T19:42:25Z

core/src/main/scala/com/salesforce/op/stages/impl/preparators/DerivedFeatureFilterUtils.scala

@@ -361,6 +373,10 @@ private[op] case class ColumnStatistics
 corrLabel.filter(Math.abs(_) > maxCorrelation).map(corr =>
 s"correlation $corr higher than max correlation $maxCorrelation"
 ),
+ column.flatMap{ case cl => featureCorrs.take(cl.index).find(Math.abs(_) > maxFeatureCorr).map(corr =>
+ s"this feature has correlations $corr with other features higher than max feature-feature" +


Semantics but would: "with another feature higher than" be better?

core/src/main/scala/com/salesforce/op/stages/impl/preparators/SanityChecker.scala

nicodv · 2020-05-13T19:45:16Z

core/src/main/scala/com/salesforce/op/stages/impl/preparators/SanityChecker.scala

 )
 def setMaxCorrelation(value: Double): this.type = set(maxCorrelation, value)
 def getMaxCorrelation: Double = $(maxCorrelation)

+ final val maxFeatureCorr = new DoubleParam(
+ parent = this, name = "maxFeatureCorr",
+ doc = "Maximum correlation (absolute value) allowed between a feature two feature vectors which will" +


typo: "a feature two feature vectors"

nicodv · 2020-05-13T19:52:21Z

core/src/main/scala/com/salesforce/op/stages/impl/preparators/SanityCheckerMetadata.scala

 corrType: CorrelationType
 ) extends MetadataLike {
- require(featuresIn.length == values.length, "Feature names and correlation values arrays must have the same length")
+ require(featuresIn.length == valuesWithLabel.length,


to be defensive, maybe add a require for a square correlation matrix? (or later in the code, as it may not be calculated)

correlation matrix may be empty so not sure it makes sense

nicodv · 2020-05-13T20:03:53Z

core/src/main/scala/com/salesforce/op/stages/impl/preparators/SanityCheckerMetadata.scala

- corrType = CorrelationType.withNameInsensitive(wrapped.get[String](SanityCheckerNames.CorrelationType))
- )
+ val features = wrapped.getArray[String](SanityCheckerNames.FeaturesIn).toSeq
+ if (wrapped.underlyingMap.keySet.contains("values")) { // old sanity checker meta


nice backwards compatibility hack! maybe use "correlationsWithLabel" instead of "values", as its speaks more to the type of change that occurred?

nicodv · 2020-05-14T20:35:36Z

LGTM

Jauntbox · 2020-05-14T22:00:53Z

core/src/main/scala/com/salesforce/op/stages/impl/preparators/SanityChecker.scala

@@ -94,15 +94,24 @@ trait SanityCheckerParams extends DerivedFeatureFilterParams {
 final val maxCorrelation = new DoubleParam(
 parent = this, name = "maxCorrelation",
 doc = "Maximum correlation (absolute value) allowed between a feature in the feature vector and the label",
- isValid = ParamValidators.inRange(lowerBound = 0.0, upperBound = 1.0, lowerInclusive = true, upperInclusive = true)
+ isValid = ParamValidators.inRange(lowerBound = -0.1, upperBound = 1.1, lowerInclusive = true, upperInclusive = true)


Why did these ranges need to be changed? We shouldn't get correlations outside of [0, 1], right?

Jauntbox · 2020-05-14T22:01:28Z

core/src/main/scala/com/salesforce/op/stages/impl/preparators/SanityChecker.scala

 final val minCorrelation = new DoubleParam(
 parent = this, name = "minCorrelation",
 doc = "Minimum correlation (absolute value) allowed between a feature in the feature vector and the label",
- isValid = ParamValidators.inRange(lowerBound = 0.0, upperBound = 1.0, lowerInclusive = true, upperInclusive = true)
+ isValid = ParamValidators.inRange(lowerBound = -0.1, upperBound = 1.1, lowerInclusive = true, upperInclusive = true)


Same here - why are these changed?

Jauntbox · 2020-05-14T22:03:45Z

core/src/main/scala/com/salesforce/op/stages/impl/preparators/SanityCheckerMetadata.scala

- stats.filter(s => s.corrLabel.isDefined && !s.corrLabel.get.isNaN).map(s => s.name -> s.corrLabel.get),
- stats.filter(s => s.corrLabel.isDefined && s.corrLabel.get.isNaN).map(_.name),
+ correlations = new Correlations(
+ stats.filter(s => s.corrLabel.isDefined).map(s => (s.name, s.corrLabel.get, s.featureCorrs)),


Do we not have to check for NaNs anymore? What if the feature is constant and we turn off the min variance threshold?

So I fixed it - in that when it goes to metadata it turns it into strings and then when it gets out back into the class it is turned into doubles. Now nans work! The json serializer would not have helped because it was a problem with the spark metadata.

Jauntbox · 2020-05-14T23:18:49Z

core/src/main/scala/com/salesforce/op/stages/impl/preparators/SanityCheckerMetadata.scala

- * @param values correlation of feature with label
- * @param nanCorrs nan correlation features
+ * @param valuesWithLabel correlation of feature with label
+ * @param valuesWithFeatures correlations between features features


"correlations between features ~~features~~"

Jauntbox · 2020-05-14T23:50:42Z

core/src/test/scala/com/salesforce/op/stages/impl/preparators/BadFeatureZooTest.scala

@@ -88,6 +88,7 @@ class BadFeatureZooTest extends FlatSpec with TestSparkContext with Logging {
 val genFeatureVector = Seq(rawCity, rawCountry, rawPickList, rawCurrency).transmogrify()
 val checkedFeatures = new SanityChecker()
 .setCheckSample(1.0)
+ .setMaxFeatureCorr(1.1)


Oh, I see - you're using values > 1.0 to turn it off. I think it may be clearer if we keep the bounds in [0, 1.0], but make it a < check instead of a <= check (so that setting it to 1.0 would turn it off).

So I did that! Turns out that with doubles you can get a correlation > 1.0 with perfectly correlated things :-P

Eg in a couple of test 1 had it set to 1.0 with corr > and it failed because the correlation was 1.00000000004

…nto lm/removeDups

…removeDups

gerashegalov · 2020-05-19T07:53:38Z

core/src/main/scala/com/salesforce/op/stages/impl/preparators/SanityChecker.scala

 )
 def setMaxCorrelation(value: Double): this.type = set(maxCorrelation, value)
 def getMaxCorrelation: Double = $(maxCorrelation)

+ final val maxFeatureCorr = new DoubleParam(


better spell out Correlation in a public parameter

salesforce-cla · 2020-12-09T12:18:40Z

Thanks for the contribution! Unfortunately we can't verify the commit author(s): leahmcguire <l***@s***.com> Leah McGuire <l***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, refresh the status of this Pull Request.

leahmcguire added 5 commits May 7, 2020 14:59

use the full corr matrix'

58011f0

fixed some tests

b1dca9c

fixed more tests

0449f48

added test

ce35545

added metadata test

85f2159

leahmcguire requested review from gerashegalov, Jauntbox, tovbinm and wsuchy as code owners May 11, 2020 16:16

nicodv suggested changes May 13, 2020

View reviewed changes

leahmcguire added 2 commits May 14, 2020 10:20

addressing comments

04f10b6

Merge branch 'master' into lm/removeDups

550979e

nicodv approved these changes May 14, 2020

View reviewed changes

Jauntbox reviewed May 14, 2020

View reviewed changes

leahmcguire added 3 commits May 18, 2020 16:17

fixed typo

fffbfdc

Merge branch 'lm/removeDups' of github.com:salesforce/TransmogrifAI i…

42dda4e

…nto lm/removeDups

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

8340c28

…removeDups

gerashegalov reviewed May 19, 2020

View reviewed changes

leahmcguire added 2 commits May 19, 2020 11:10

changing param name

c86f8af

chaging name

ac1c3a7

leahmcguire merged commit d222438 into master May 19, 2020

nicodv mentioned this pull request Jun 11, 2020

0.7.0 release #481

Merged

tovbinm deleted the lm/removeDups branch June 12, 2020 01:33

salesforce-cla bot added the cla:missing label Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove duplicate features using sanity checker feature to feature correlations #476

Remove duplicate features using sanity checker feature to feature correlations #476

leahmcguire commented May 11, 2020

codecov bot commented May 11, 2020 •

edited

Loading

nicodv left a comment

nicodv May 13, 2020

nicodv May 13, 2020

nicodv May 13, 2020

nicodv May 13, 2020

nicodv May 13, 2020

leahmcguire May 13, 2020

nicodv May 13, 2020

nicodv commented May 14, 2020

Jauntbox May 14, 2020 •

edited

Loading

Jauntbox May 14, 2020

Jauntbox May 14, 2020

leahmcguire May 15, 2020

Jauntbox May 14, 2020

Jauntbox May 14, 2020

leahmcguire May 15, 2020

gerashegalov May 19, 2020 •

edited

Loading

salesforce-cla bot commented Dec 9, 2020

Remove duplicate features using sanity checker feature to feature correlations #476

Remove duplicate features using sanity checker feature to feature correlations #476

Conversation

leahmcguire commented May 11, 2020

codecov bot commented May 11, 2020 • edited Loading

Codecov Report

nicodv 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

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

nicodv commented May 14, 2020

Jauntbox May 14, 2020 • edited Loading

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

gerashegalov May 19, 2020 • edited Loading

Choose a reason for hiding this comment

salesforce-cla bot commented Dec 9, 2020

codecov bot commented May 11, 2020 •

edited

Loading

Jauntbox May 14, 2020 •

edited

Loading

gerashegalov May 19, 2020 •

edited

Loading