Possibility to Return top K positives and top K negatives for LOCO #264

michaelweilsalesforce · 2019-04-03T20:50:57Z

Describe the proposed solution

New feature used only for Binary Classification (probability of predicting a positive label) and Regression.

The map's contents are different regarding the value of the topKStrategy param :
If PositiveNegative, returns at most 2 x topK elements : the topK most positive and the topK most negative derived features based on the LOCO insight.
If Abs, returns at most topK elements : the topK derived features having the highest absolute value of LOCO score.

codecov · 2019-04-03T21:17:02Z

Codecov Report

Merging #264 into master will increase coverage by 0.02%.
The diff coverage is 97.82%.

@@            Coverage Diff             @@
##           master     #264      +/-   ##
==========================================
+ Coverage   86.63%   86.66%   +0.02%     
==========================================
  Files         315      315              
  Lines       10366    10396      +30     
  Branches      336      556     +220     
==========================================
+ Hits         8981     9010      +29     
- Misses       1385     1386       +1

Impacted Files	Coverage Δ
...e/op/stages/impl/insights/RecordInsightsLOCO.scala	`95.31% <97.82%> (+1.19%)`	⬆️

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 209647c...a137d61. Read the comment docs.

tovbinm · 2019-04-04T04:29:33Z

core/src/main/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCO.scala

+ * derived features based on the LOCO insight.
+ * - If Abs, returns at most topK elements : the topK derived features having highest absolute value of LOCO score.
+ * @param model model instance that you wish to explain
+ * @param uid   uid for instance
 */
 @Experimental


should we remove the @Experimental flag already?

no - we want to keep it so we can change things :-) For instance the way of creating this class is still terrible

tovbinm · 2019-04-04T04:30:15Z

core/src/main/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCO.scala

+      "Classification and Regression."
+  )
+
+  def setTopKStrategy(strat: TopKStrategy): this.type = set(topKStrategy, strat.entryName)


please name fully - strategy: TopKStrategy

tovbinm · 2019-04-04T04:32:43Z

core/src/main/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCO.scala

+    case 1 => ProblemType.Regression
+    case 2 => ProblemType.BinaryClassification
+    case n if (n > 2) => {
+      log.info("MultiClassification Problem : Top K LOCOs by absolute value")


This is not a very user friendly message. Let's log some meaningful message for any problem type with some clear and comprehensible explanation.

tovbinm · 2019-04-04T04:36:19Z

core/src/main/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCO.scala

-      featureArray.update(i, (oldInd, oldVal))
-      i += 1
+    val top = $(topKStrategy) match {
+      case s if s == TopKStrategy.Abs.entryName || problemType == ProblemType.MultiClassification => {


add a getter method for strategy def getTopKStrategy: TopKStrategy and then use it here instead of string equality: $(topKStrategy) -> problemType match { case (TopKStrategy.Abs, ProblemType.MultiClassification) => ... }

please also create a function for each of the cases to avoid this method growing large

tovbinm · 2019-04-04T04:38:40Z

core/src/main/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCO.scala

+        var positiveCount = 0
+        // Size of negative heap
+        var negativeCount = 0
+        for {i <- 0 until filledSize} {


use while - it's faster

tovbinm · 2019-04-04T04:39:39Z

core/src/main/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCO.scala

+            if (positiveCount > k) { // remove the lowest element if the heap size goes from 5 to 6
+              positiveMaxHeap.dequeue()
+            }
+          } else if (max < 0.0) { // if negative LOCO then add it to negative heap


what about max == 0.0? it's not handled.

No. LOCOs of 0.0 are not interesting

tovbinm · 2019-04-04T04:40:20Z

core/src/main/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCO.scala

+        for {i <- 0 until filledSize} {
+          val (oldInd, oldVal) = featureArray(i)
+          val diffs = computeDiffs(i, oldInd, featureArray, featureSize, baseScore)
+          val max = if (problemType == ProblemType.Regression) diffs(0) else diffs(1)


this seems very hacky and error prone - if (problemType == ProblemType.Regression) diffs(0) else diffs(1), perhaps add a helper function?

this is where you would use the indexToExamine variable I suggested above

tovbinm · 2019-04-04T04:43:21Z

core/src/test/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCOTest.scala

+    val parsed = insights.collect(name, transformer.getOutput())
+      .map { case (n, i) => n -> RecordInsightsParser.parseInsights(i) }
+    parsed.foreach { case (_, in) =>
+      in.head._1.columnName == "1_1_1_1" || in.last._1.columnName == "3_3_3_3" shouldBe true


what does this even mean in.head._1.columnName == "1_1_1_1" || in.last._1.columnName == "3_3_3_3"?
please add a withClue around it to allow easier debugging in case of failures.

tovbinm · 2019-04-04T04:44:32Z

core/src/test/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCOTest.scala

+    val label = labelNoRes.copy(isResponse = true)
+    val testDataMeta = addMetaData(testData, "features", 5)
+    val sparkModel = new OpLogisticRegression().setInput(label, featureVector).fit(testData)
+


remove redundant lines

leahmcguire · 2019-04-04T15:38:17Z

core/src/main/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCO.scala

+    case 0 => ProblemType.Unknown
+    case 1 => ProblemType.Regression
+    case 2 => ProblemType.BinaryClassification
+    case n if (n > 2) => {


I was thinking about the multiclass issue - I think it makes sense to return the difference in score for the winning category so maybe instead of having this find the problem type it should return the index to examine (also move it down to the base score function line 117:

val baseResult = modelApply(labelDummy, features)
val baseScore = baseResult.score
val indexToExamine = baseScore.length match {
case 0 => throw new RuntimeExeption("model does not produce scores for insights")
case 1 => 0
case 2 => 1
case n if (n > 2) => baseResult.prediction

and then always use this index for pulling out the diff for the heaps

leahmcguire · 2019-04-04T15:48:10Z

core/src/main/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCO.scala

+        topAbs.sortBy { case (_, v, _) => -math.abs(v) }
+
+      }
+      case s if s == TopKStrategy.PositiveNegative.entryName => {


what if we always kept the positive and negative heap and then just check the strategy when we return the insights?

I like that! This would keep the code simpler.

It would be slower when we want to return the top by abs value. Is there an easy way to go from 2 Priority Queues to 1?

It would be slightly slower but not by much if we assume that (or enforce that) k is small. To go to one q - just get all the answers and sort them. Again you are right that it is slower but if we limit k to ~100 it is a pretty small difference.

Or you can get the first of each que and then basically keep popping off the last q that you took from for comparison - then it is not much slower you just have a sightly larger memory footprint

Another issue @leahmcguire. For Multiclassification, we return the highest absolute LOCO value, and for the other strategy we look at the winning category (baseScore). Getting the top Positive and negative for winning class doesn't guarantee the highest absolute value : what if this absolute value was from another class?

Yeah - I think that the fact that we were just taking the highest absolute value was a mistake. It allows for things like baseScore = (0.1, 0.3, 0.6) new score = (0.3, 0.1, 0.6) to be the difference we report, which seems wrong. Always choosing the change in the winning value will mean that the change we report always has to do with the score that won, even if it is not the largest change in score. So yes we would change strategy for multiclass in absolute value as well - but I think it is slightly more principled...WDYT?

Now the question is should we apply the same logic for Binary Classification.
So far the spikes were focused on the positive class (label 1.0) because we thought it was the the most interesting info to report.

However, recent spikes witness a strong relationship between LOCO scores and prediction scores (cc @crupley) : Rows with the "highest scores" had on average its top absolute LOCOs being mostly positive. For "lowest scores", those locos are mostly negative.
One can also argue that LOCO for class 0 = - LOCO for class 1, hence getting top Positives and Negatives is sufficient.
Maybe something we might need to add to the output is the base prediciton.

Hmm, that is interesting. But wouldn't it be confusing to interpret for Binary if we choose the winning value because they are symetric? the scores will always some to 1 so a positive contribution to the prediction of 1 will be the same negative contribution to the prediction of 0. If we switch based on the prediction they will have to keep in mind the winning prediction to interpret the effect - this is something you need to do in multiclass but seems like an extra complication in binary.

…ransmogrifAI into mw/LOCO-improvement

leahmcguire · 2019-04-04T22:19:08Z

core/src/main/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCO.scala

+    while (i < filledSize) {
+      val (oldInd, oldVal) = featureArray(i)
+      val diffs = computeDiffs(i, oldInd, featureArray, featureSize, baseScore)
+      val max = diffs(indexToExamine)


maybe rename to diffToExamine

leahmcguire

just minor stuff then LGTM

leahmcguire · 2019-04-04T22:19:17Z

core/src/main/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCO.scala

+          negativeMaxHeap.dequeue()
+        } // Not keeping LOCOs with value 0
+      }
+      featureArray.update(i, (oldInd, oldVal))


would it make sense to move the update of the array back tot the old version into the compute diffs function?

leahmcguire · 2019-04-04T22:21:02Z

core/src/main/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCO.scala

+  override def transformFn: OPVector => TextMap = (features) => {
+    val baseResult = modelApply(labelDummy, features)
+    val baseScore = baseResult.score
+      modelApply(labelDummy, features).prediction


think line 148 is leftover from a refactor

tovbinm · 2019-04-05T02:51:04Z

core/src/main/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCO.scala

+      case n if (n > 2) => baseResult.prediction.toInt
+    }
+    val topPosNeg = returnTopPosNeg(filledSize, featureArray, featureSize, baseScore, k, indexToExamine)
+    val top = getTopKStrategy match {


getTopKStrategy match { case TopKStrategy.Abs => case TopKStrategy.PositiveNegative => }

tovbinm

lgtm, one minor comment on match statement

shenzgang · 2019-10-09T03:31:44Z

hello!How does the multi-category evaluator scale to compute roc,pr, and FPR and TPR for each category？

mweilsalesforce added 4 commits April 3, 2019 09:42

TopK Strategy Param

84e2387

Changing TransformFn

9fa76e5

Fix tests

9bce44f

Add more tests

128f62a

michaelweilsalesforce requested review from leahmcguire and tovbinm as code owners April 3, 2019 20:50

Merge branch 'master' into mw/LOCO-improvement

0a284bd

salesforce-cla bot added the cla:signed label Apr 3, 2019

michaelweilsalesforce added the ready for review label Apr 3, 2019

Merge branch 'master' into mw/LOCO-improvement

aea94de

tovbinm reviewed Apr 4, 2019

View reviewed changes

leahmcguire reviewed Apr 4, 2019

View reviewed changes

mweilsalesforce added 8 commits April 4, 2019 10:16

strat -> strategy

97bb413

Merge branch 'mw/LOCO-improvement' of https://github.com/salesforce/T…

2ef84ee

…ransmogrifAI into mw/LOCO-improvement

getTopKStrategy + method for each strat

0cec1c9

use while - it's faster

756cf0c

Removing redundant lines

f3de950

IndexToExamine

19a9199

posNeg then Absolute

ea0069d

Adding withClue

fba47ef

leahmcguire reviewed Apr 4, 2019

View reviewed changes

diffToExamine

21a653f

leahmcguire reviewed Apr 4, 2019

View reviewed changes

Minor changes

dac78ba

tovbinm reviewed Apr 5, 2019

View reviewed changes

tovbinm approved these changes Apr 5, 2019

View reviewed changes

Nicer Pattern match

a137d61

michaelweilsalesforce merged commit 40b51c7 into master Apr 5, 2019

michaelweilsalesforce deleted the mw/LOCO-improvement branch April 5, 2019 19:53

tovbinm mentioned this pull request Apr 10, 2019

Release 0.5.2 #277

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Possibility to Return top K positives and top K negatives for LOCO #264

Possibility to Return top K positives and top K negatives for LOCO #264

michaelweilsalesforce commented Apr 3, 2019

codecov bot commented Apr 3, 2019 •

edited

Loading

tovbinm Apr 4, 2019

leahmcguire Apr 4, 2019

tovbinm Apr 4, 2019

tovbinm Apr 4, 2019

tovbinm Apr 4, 2019

tovbinm Apr 4, 2019

tovbinm Apr 4, 2019

tovbinm Apr 4, 2019

michaelweilsalesforce Apr 4, 2019

tovbinm Apr 4, 2019

leahmcguire Apr 4, 2019

tovbinm Apr 4, 2019

tovbinm Apr 4, 2019

leahmcguire Apr 4, 2019

leahmcguire Apr 4, 2019

tovbinm Apr 4, 2019

michaelweilsalesforce Apr 4, 2019

leahmcguire Apr 4, 2019

leahmcguire Apr 4, 2019

michaelweilsalesforce Apr 4, 2019

leahmcguire Apr 4, 2019 •

edited

Loading

michaelweilsalesforce Apr 4, 2019 •

edited

Loading

leahmcguire Apr 4, 2019

leahmcguire Apr 4, 2019

leahmcguire left a comment

leahmcguire Apr 4, 2019

leahmcguire Apr 4, 2019

tovbinm Apr 5, 2019

tovbinm left a comment

shenzgang commented Oct 9, 2019

Possibility to Return top K positives and top K negatives for LOCO #264

Possibility to Return top K positives and top K negatives for LOCO #264

Conversation

michaelweilsalesforce commented Apr 3, 2019

codecov bot commented Apr 3, 2019 • edited Loading

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

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

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 Apr 4, 2019 • edited Loading

Choose a reason for hiding this comment

michaelweilsalesforce Apr 4, 2019 • edited Loading

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

Choose a reason for hiding this comment

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

shenzgang commented Oct 9, 2019

codecov bot commented Apr 3, 2019 •

edited

Loading

leahmcguire Apr 4, 2019 •

edited

Loading

michaelweilsalesforce Apr 4, 2019 •

edited

Loading