Aggregate LOCOs of DateToUnitCircleTransformer. #349
Conversation
|
Thanks for the contribution! It looks like @ijeri is an internal user so signing the CLA is not required. However, we need to confirm this. |
|
Thanks for the contribution! Before we can merge this, we need @sanmitra to sign the Salesforce.com Contributor License Agreement. |
Codecov Report
@@ Coverage Diff @@
## master #349 +/- ##
==========================================
+ Coverage 86.8% 86.83% +0.03%
==========================================
Files 336 336
Lines 10865 10873 +8
Branches 367 576 +209
==========================================
+ Hits 9431 9442 +11
+ Misses 1434 1431 -3
Continue to review full report at Codecov.
|
core/src/main/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCO.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCOTest.scala
Outdated
Show resolved
Hide resolved
|
|
||
| private def isMapFeature(featureType: String ): Boolean = { | ||
| val featureTypeTag = FeatureType.featureTypeTag(featureType) | ||
| featureTypeTag.tpe <:< weakTypeOf[OPMap[_]] |
There was a problem hiding this comment.
this check is very very very slow to perform at runtime. let's just check if history.grouping is present or not. ie.
private def getRawFeatureName(history: OpVectorColumnHistory): Option[String] = history.grouping match {
case Some(grouping) => history.parentFeatureOrigins.headOption.map(_ + "_" + grouping)
case None => history.parentFeatureOrigins.headOption
}| for {name <- getRawFeatureName(history)} { | ||
| // Update the aggregation map for each (rawFeatureName, timePeriod) in case of date features. | ||
| val key = if (isUnitCircleDateFeature) { | ||
| name + "_" + history.descriptorValue.flatMap(convertToTimePeriod).map(_.entryName).getOrElse("") |
There was a problem hiding this comment.
let's replace isUnitCircleDateFeature val and if / else with pattern matching
There was a problem hiding this comment.
pattern match on what exactly ? history ?
There was a problem hiding this comment.
btw, isUnitCircleDateFeature is referred multiple times, hence created a val.
There was a problem hiding this comment.
yes, you can probably pattern match on history and it should make the code more readable as you would have clearly separated cases. give it a try.
There was a problem hiding this comment.
and remember that you can pattern match together with if conditions.
| @@ -466,4 +427,167 @@ class RecordInsightsLOCOTest extends FlatSpec with TestSparkContext { | |||
| assertAggregatedTextMap(textAreaMap, "k1") | |||
|
|
|||
| } | |||
| it should "aggregate values for date, datetime, dateMap and dateTimeMap derived features" in { | |||
There was a problem hiding this comment.
sorry, I am not able to follow these tests anymore.
Can we please make them more readable by either splitting them into smaller test cases or rather create a behavior (http://www.scalatest.org/user_guide/sharing_tests), or at least add clues withClue - http://www.scalatest.org/user_guide/using_assertions
There was a problem hiding this comment.
sure, will refractor.
|
@tovbinm please take a look at the refactoring of test. Thanks. |
core/src/main/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCO.scala
Outdated
Show resolved
Hide resolved
…mogrifAI into san/dateLOCOAggregation
|
Thanks for the contribution! Unfortunately we can't verify the commit author(s): leahmcguire <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. |
Related issues
Aggregated LOCOs of DateToUnitCircleTransformer outputs.
Describe the proposed solution
For each time period we aggregate all the LOCOs from the same date feature by the mean.