Integrate Streaming Histogram into RawFeatureFilter. #179
Conversation
…histogram-integration
…histogram-integration
…histogram-integration
marcovivero
changed the title
Codecov Report
@@ Coverage Diff @@
## master #179 +/- ##
==========================================
+ Coverage 86.36% 86.38% +0.01%
==========================================
Files 310 311 +1
Lines 10136 10304 +168
Branches 351 553 +202
==========================================
+ Hits 8754 8901 +147
- Misses 1382 1403 +21
Continue to review full report at Codecov.
|
…smogrifAI into mv/histogram-integration
| def plus(l: TextSummary, r: TextSummary): TextSummary = l.merge(r) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
could you add some comments for the newly added classes, is this for all numerics? (compared to textSummary)?
|
is there a easy way for us to fall back to original simple binning after this change? |
| @@ -64,3 +75,162 @@ case object Summary { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| class TextSummary(textFormula: TextSummary => Int) extends Serializable { | |||
There was a problem hiding this comment.
I'm quite confused here, is old Summary class still in use? or it's completely replaced by the new ones (text summary and numeric summary)
There was a problem hiding this comment.
Summary class seems no longer needed.
| if (points.nonEmpty) key -> summary.update(points) else key -> summary | ||
| }.toMap | ||
|
|
||
| def updateTextSummaries( |
There was a problem hiding this comment.
how would text bins formula be passed in to this?
There was a problem hiding this comment.
I think we should add textBinsFormula as input argument to both getAllSummaries and updateTextSummaries.
|
|
||
| val scoringUnfilled = | ||
| if (scoringDistribs.nonEmpty) { | ||
| require(scoringDistribs.length == featureSize, "scoring and training features must match") |
There was a problem hiding this comment.
what will happen if scoring Distribution is empty or feature size does not match now, is it checked somewhere?
There was a problem hiding this comment.
I think it's better to separate scoringUnfilled check out from distribMismatches check in getAllReasons.
| @@ -38,15 +38,15 @@ object DefaultSelectorParams { | |||
| val MaxBin = Array(32) // bins for cont variables in trees - 32 is spark default | |||
| val MinInstancesPerNode = Array(10, 100) // spark default 1 | |||
| val MinInfoGain = Array(0.001, 0.01, 0.1) // spark default 0 | |||
| val Regularization = Array(0.001, 0.01, 0.1, 0.2) // spark default 0 | |||
| val Regularization = Array(0.0001, 0.0015, 0.001, 0.01, 0.1) // spark default 0 | |||
There was a problem hiding this comment.
why did we make those hyper-parameter change, are there any related experiments?
There was a problem hiding this comment.
The only related doc I can find: https://salesforce.quip.com/JhIiA1PLIYUY (streaming histogram results)
| val (totalCount, responseSummaries, numericSummaries, textSummaries) = sum | ||
| val (responseFeatures, numericFeatures, textFeatures) = feat | ||
|
|
||
| def updateNumericSummaries( |
There was a problem hiding this comment.
this might be just my personal feelings, is this nested too much? e.g. three levels nested method
| } | ||
|
|
||
| // This will initialize existing text summary hashing TFs | ||
| textSummaries.foreach { case (_, textSum) => textSum.setHashingTF() } |
There was a problem hiding this comment.
Perhaps call setHashingTF when creating a new TextSummary (i.e., in updateTextSummaries)?
| .getOrElse(key -> summary) | ||
| }.toMap | ||
|
|
||
| val newResponseSummaries = updateNumericSummaries(responseSummaries, responseFeatures) |
…ansmogrifAI into mv/histogram-integration
…histogram-integration
|
@marcovivero are you not going to work on this or you are making a clean / smaller one? |
Related issues
No related issue, this is an enhancement.
Describe the proposed solution
Integrating StreamingHistogram with histogram generation which happens in RawFeatureFilter. Text distribution computation has also been separated from numeric distribution computations.