Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Descale feature contribution for Linear Regression & Logistic Regression #345

Merged
merged 43 commits into from
Jul 25, 2019
Merged

Descale feature contribution for Linear Regression & Logistic Regression #345

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
fe8e6df
move PR to a branch on Tmog
TuanNguyen27 Jun 25, 2019
4ad6204
add condition to descale only when it is Linear / Logistic regression…
TuanNguyen27 Jun 25, 2019
1e2bc81
fix modelinsighttest
TuanNguyen27 Jun 25, 2019
5cbe132
only compute descaled contrib if a model is present & fits our criteria
TuanNguyen27 Jun 25, 2019
673dad8
fix test failure with a check for empty list of feature contribution
TuanNguyen27 Jun 25, 2019
4c44263
addressing comments
TuanNguyen27 Jun 27, 2019
d3fa01b
more comment addressing
TuanNguyen27 Jun 27, 2019
7a882f2
test in progress, still broken
TuanNguyen27 Jun 27, 2019
8ba36c2
Merge branch 'master' into tn/descaleLR
tovbinm Jun 27, 2019
e4da5ab
seems to be working
TuanNguyen27 Jun 27, 2019
0767e84
first version of test
TuanNguyen27 Jun 28, 2019
992db1a
Merge branch 'tn/descaleLR' of https://github.com/salesforce/Transmog…
TuanNguyen27 Jun 28, 2019
7eaa209
fix scala style
TuanNguyen27 Jun 28, 2019
2c5d2f7
Merge branch 'master' into tn/descaleLR
tovbinm Jul 2, 2019
99236f4
Merge branch 'master' into tn/descaleLR
TuanNguyen27 Jul 3, 2019
e1bb482
addressing comments
TuanNguyen27 Jul 8, 2019
00fb0d6
Merge branch 'master' into tn/descaleLR
TuanNguyen27 Jul 8, 2019
27a2449
Merge branch 'master' into tn/descaleLR
TuanNguyen27 Jul 9, 2019
cb798db
change log to warn
TuanNguyen27 Jul 9, 2019
3a541ed
Merge branch 'master' into tn/descaleLR
leahmcguire Jul 11, 2019
083448a
Merge branch 'master' into tn/descaleLR
leahmcguire Jul 11, 2019
606b6e1
fix an error in calculating standard deviation for discrete distribution
TuanNguyen27 Jul 11, 2019
1643635
Merge branch 'tn/descaleLR' of https://github.com/salesforce/Transmog…
TuanNguyen27 Jul 11, 2019
6ca53fc
Merge branch 'master' into tn/descaleLR
leahmcguire Jul 11, 2019
4c432e6
correctly pull out standard deviation for each feature
TuanNguyen27 Jul 11, 2019
4f252bd
Merge branch 'master' into tn/descaleLR
TuanNguyen27 Jul 12, 2019
4be8752
Merge branch 'master' into tn/descaleLR
TuanNguyen27 Jul 12, 2019
0e63491
descale entire contribution vector & clearly separate out between lin…
TuanNguyen27 Jul 12, 2019
e6de82b
Merge branch 'tn/descaleLR' of https://github.com/salesforce/Transmog…
TuanNguyen27 Jul 12, 2019
54d28a1
Merge branch 'master' into tn/descaleLR
TuanNguyen27 Jul 14, 2019
4677c96
fix scala idiom
TuanNguyen27 Jul 15, 2019
fa4221f
Merge branch 'tn/descaleLR' of https://github.com/salesforce/Transmog…
TuanNguyen27 Jul 15, 2019
a5901d8
remove logistic regression pattern matching
TuanNguyen27 Jul 15, 2019
90ff504
add citations for future readability
TuanNguyen27 Jul 15, 2019
a7dea4e
refactor & add test for binary logistic regression case
TuanNguyen27 Jul 18, 2019
35bdfe8
Merge branch 'master' into tn/descaleLR
TuanNguyen27 Jul 18, 2019
c6bae48
remove redundant import
TuanNguyen27 Jul 18, 2019
bc60187
fix scala style
TuanNguyen27 Jul 19, 2019
a6839b2
fix scala style again
TuanNguyen27 Jul 19, 2019
23b2443
Update warning message
TuanNguyen27 Jul 22, 2019
36c8420
update failure threshold so test will pass
TuanNguyen27 Jul 22, 2019
c80cc1a
update test to be ratio instead of absolute difference
TuanNguyen27 Jul 24, 2019
51627e9
small update to set tolerance threshold
TuanNguyen27 Jul 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions core/src/main/scala/com/salesforce/op/ModelInsights.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ case object ModelInsights {
ModelInsights(
label = getLabelSummary(label, checkerSummary),
features = getFeatureInsights(vectorInput, checkerSummary, model, rawFeatures,
blacklistedFeatures, blacklistedMapKeys, rawFeatureFilterResults),
blacklistedFeatures, blacklistedMapKeys, rawFeatureFilterResults, getLabelSummary(label, checkerSummary)),
TuanNguyen27 marked this conversation as resolved.
Show resolved Hide resolved
selectedModelInfo = getModelInfo(model),
trainingParams = trainingParams,
stageInfo = RawFeatureFilterConfig.toStageInfo(rawFeatureFilterResults.rawFeatureFilterConfig)
Expand Down Expand Up @@ -537,7 +537,8 @@ case object ModelInsights {
rawFeatures: Array[features.OPFeature],
blacklistedFeatures: Array[features.OPFeature],
blacklistedMapKeys: Map[String, Set[String]],
rawFeatureFilterResults: RawFeatureFilterResults = RawFeatureFilterResults()
rawFeatureFilterResults: RawFeatureFilterResults = RawFeatureFilterResults(),
label: LabelSummary
): Seq[FeatureInsights] = {
val featureInsights = (vectorInfo, summary) match {
case (Some(v), Some(s)) =>
Expand All @@ -557,6 +558,25 @@ case object ModelInsights {
case _ => None
}
val keptIndex = indexInToIndexKept.get(h.index)
val featureStd = getIfExists(h.index, s.featuresStatistics.variance).getOrElse(1.0)
TuanNguyen27 marked this conversation as resolved.
Show resolved Hide resolved
val sparkFtrContrib = keptIndex
.map(i => contributions.map(_.applyOrElse(i, (_: Int) => 0.0))).getOrElse(Seq.empty)
val LRStandardization = checkLRStandardization(model).getOrElse(false)
val labelStd = label.distribution.getOrElse(1.0) match {
TuanNguyen27 marked this conversation as resolved.
Show resolved Hide resolved
case Continuous(_, _, _, variance) => math.sqrt(variance)
// for (binary) logistic regression we only need to multiply by feature standard deviation
case Discrete(domain, prob) =>
def computeVariance(domain: Seq[String], prob: Seq[Double]): Double = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you dont need the inner function and you can do it faster. simply do:

case Discrete(domain, prob) =>
   val (weighted, sqweighted) = (domain zip prob).foldLeft((0.0, 0.0)) { case ((weightSum, sqweightSum), (d, p)) =>
         val floatD = d.toDouble
         val weight = floatD * prob
         val sqweight = floatD * weight
         (weightSum+ weight, sqweightSum + sqweight)  
   }
   sqweighted - weighted

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another question, since domain is Seq[String] how can we be sure that d.toDouble wont throw an error?! should we handle it gracefully?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For regression problem where there are too few unique labels (and they get treated as categoricals), d.toDouble should work fine. I'm not sure how it will behave on classification. For classification I assume that domain = Array("0", "1"), is this correct ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we decide to define domain as Seq[String] in the first place? @Jauntbox @leahmcguire

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have the raw label rather than the indexed label you will get strings - this was designed to support that

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it safe to do toDouble in this case ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any value which is not numeric with throw an exception, e.g. "".toDouble

Copy link
Collaborator Author

@TuanNguyen27 TuanNguyen27 Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but will there ever be non numeric string in the label field...? i thought they should be filtered out.

val floatDomain = domain.map(_.toDouble)
val sqfloatDomain = floatDomain.map(math.pow(_, 2))
val weighted = (floatDomain, prob).zipped map (_ * _)
val sqweighted = (sqfloatDomain, prob).zipped map (_ * _)
val mean = weighted.sum
return sqweighted.sum - mean
}
computeVariance(domain, prob)
}
// TODO: throw exception if (labelStd == 0)
TuanNguyen27 marked this conversation as resolved.
Show resolved Hide resolved

h.parentFeatureOrigins ->
Insights(
Expand All @@ -579,7 +599,10 @@ case object ModelInsights {
case _ => Map.empty[String, Double]
},
contribution =
keptIndex.map(i => contributions.map(_.applyOrElse(i, (_: Int) => 0.0))).getOrElse(Seq.empty),
if (LRStandardization && sparkFtrContrib.nonEmpty) {
sparkFtrContrib.updated(0, sparkFtrContrib.head * featureStd / labelStd)
TuanNguyen27 marked this conversation as resolved.
Show resolved Hide resolved
}
else sparkFtrContrib,
min = getIfExists(h.index, s.featuresStatistics.min),
max = getIfExists(h.index, s.featuresStatistics.max),
mean = getIfExists(h.index, s.featuresStatistics.mean),
Expand Down Expand Up @@ -647,6 +670,18 @@ case object ModelInsights {
}
}

private[op] def checkLRStandardization(model: Option[Model[_]]): Option[Boolean] = {
val stage = model.flatMap {
TuanNguyen27 marked this conversation as resolved.
Show resolved Hide resolved
case m: SparkWrapperParams[_] => m.getSparkMlStage()
case _ => None
}
stage.collect {
case m: LogisticRegressionModel => true && m.getStandardization
case m: LinearRegressionModel => true && m.getStandardization
case _ => false
}
}

private[op] def getModelContributions
(model: Option[Model[_]], featureVectorSize: Option[Int] = None): Seq[Seq[Double]] = {
val stage = model.flatMap {
Expand Down
4 changes: 3 additions & 1 deletion core/src/test/scala/com/salesforce/op/ModelInsightsTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,9 +509,11 @@ class ModelInsightsTest extends FlatSpec with PassengerSparkFixtureTest with Dou
}

it should "correctly extract the FeatureInsights from the sanity checker summary and vector metadata" in {
val labelSum = ModelInsights.getLabelSummary(Option(lbl), Option(summary))

val featureInsights = ModelInsights.getFeatureInsights(
Option(meta), Option(summary), None, Array(f1, f0), Array.empty, Map.empty[String, Set[String]],
RawFeatureFilterResults()
RawFeatureFilterResults(), labelSum
)
featureInsights.size shouldBe 2

Expand Down