Binary sequence estimator #84

mikeloh77 · 2018-08-24T05:06:43Z

It would be useful to have an Estimator/Transformer combo where it can take a single feature of type I1 and a sequence of features of type I2. This is to augment the concept of the SequenceEstimator.

salesforce-cla · 2018-08-24T05:06:46Z

Thanks for the contribution! Before we can merge this, we need @mikeloh77 to sign the Salesforce.com Contributor License Agreement.

tovbinm · 2018-08-24T06:00:34Z

features/src/main/scala/com/salesforce/op/features/FeatureSparkTypes.scala

+    val convertI1 = FeatureTypeSparkConverter[I1]()
+    val convertI2 = FeatureTypeSparkConverter[I2]()
+    val func = (r: Row) => {
+      val arr = new ArrayBuffer[I2](r.length-1)


add spaces r.length - 1

tovbinm · 2018-08-24T06:02:47Z

features/src/main/scala/com/salesforce/op/features/FeatureSparkTypes.scala

@@ -470,6 +470,46 @@ case object FeatureSparkTypes {
    }
  }

+  def udf2N[I1 <: FeatureType : TypeTag, I2 <: FeatureType : TypeTag, O <: FeatureType : TypeTag]


tovbinm · 2018-08-24T06:02:55Z

features/src/main/scala/com/salesforce/op/features/FeatureSparkTypes.scala

+    UserDefinedFunction(func, outputType, inputTypes = None)
+  }
+
+  def transform2N[I1 <: FeatureType : TypeTag, I2 <: FeatureType : TypeTag, O <: FeatureType: TypeTag]


docs here as well

tovbinm · 2018-08-24T06:10:52Z

features/src/main/scala/com/salesforce/op/stages/OpPipelineStages.scala

@@ -549,6 +549,43 @@ trait OpPipelineStageN[I <: FeatureType, O <: FeatureType] extends OpPipelineSta
  )(tto)
 }

+/**
+ * Pipeline stage of single Feature of type I1 with multiple Features of type I2 to output 1Feature of type O


typo 1Feature

tovbinm · 2018-08-24T06:13:40Z

features/src/main/scala/com/salesforce/op/stages/base/sequence/BinarySequenceEstimator.scala

+  implicit val tti1: TypeTag[I1],
+  val tti2: TypeTag[I2],
+  val tto: TypeTag[O],
+  val tti1v: TypeTag[I1#Value],


please rename tti1v -> ttiv1 and tti2v -> ttiv2 (since we have those in other classes as well) + update the docs

tovbinm · 2018-08-24T06:19:27Z

features/src/main/scala/com/salesforce/op/stages/base/sequence/BinarySequenceEstimator.scala

+    val columns = Array(col(in1.name)) ++ seqColumns
+
+    val df = dataset.select(columns: _*)
+    val ds = df.map(r => {


you should be using the converters here instead:

val ds = df.map { r => val rowSeq = r.toSeq (convertI1.fromSpark(rowSeq.head), rowSeq.tail.map(seqIConvert.fromSpark(_).value)) }

tovbinm · 2018-08-24T06:21:23Z

features/src/main/scala/com/salesforce/op/stages/base/sequence/BinarySequenceTransformer.scala

+import scala.reflect.runtime.universe.TypeTag
+import scala.util.Try
+
+trait OpTransformer2N[I1 <: FeatureType, I2 <: FeatureType, O <: FeatureType]


tovbinm · 2018-08-24T06:23:35Z

features/src/main/scala/com/salesforce/op/stages/base/sequence/BinarySequenceEstimator.scala

+   * @return a fitted model that will perform the transformation specified by the function defined in constructor fit
+   */
+  override def fit(dataset: Dataset[_]): BinarySequenceModel[I1, I2, O] = {
+    assert(inN.nonEmpty, "Inputs cannot be empty")


perhaps getTransientFeatures.nonEmpty?

this is a check specifically on the sequence input getTransientFeatures will be nonempty

then getTransientFeatures.size > 1, so it will include the check of existence of in1 as well

tovbinm · 2018-08-24T06:23:49Z

features/src/main/scala/com/salesforce/op/stages/base/sequence/BinarySequenceTransformer.scala

+   * @return a new dataset containing a column for the transformed feature
+   */
+  override def transform(dataset: Dataset[_]): DataFrame = {
+    assert(inN.nonEmpty, "Inputs cannot be empty")


perhaps getTransientFeatures.nonEmpty here as well?

tovbinm · 2018-08-24T06:32:52Z

...es/src/test/scala/com/salesforce/op/stages/base/sequence/BinarySequenceTransformerTest.scala

+    Set("-1.0", "three", "four").toMultiPickList,
+    Set("15.0", "five", "six").toMultiPickList,
+    Set("1.111", "seven", "").toMultiPickList
+  )


.map(_.toMultiPickList)

tovbinm · 2018-08-24T06:33:10Z

...ures/src/test/scala/com/salesforce/op/stages/base/sequence/BinarySequenceEstimatorTest.scala

+    Vectors.dense(0.4, 0.5, Double.PositiveInfinity).toOPVector,
+    Vectors.dense(0.4, 1.0, Double.PositiveInfinity).toOPVector,
+    Vectors.dense(0.2, 0.5, Double.PositiveInfinity).toOPVector
+  )


.map(_.toOPVector)

codecov · 2018-08-24T07:12:11Z

Codecov Report

Merging #84 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #84      +/-   ##
==========================================
+ Coverage   86.08%   86.17%   +0.09%     
==========================================
  Files         292      294       +2     
  Lines        9490     9550      +60     
  Branches      316      528     +212     
==========================================
+ Hits         8169     8230      +61     
+ Misses       1321     1320       -1

Impacted Files	Coverage Δ
...rc/main/scala/com/salesforce/op/stages/HasIn.scala	`100% <100%> (ø)`	⬆️
...com/salesforce/op/features/FeatureSparkTypes.scala	`90.62% <100%> (+0.91%)`	⬆️
...stages/base/sequence/BinarySequenceEstimator.scala	`100% <100%> (ø)`
...la/com/salesforce/op/stages/OpPipelineStages.scala	`75.45% <100%> (+2.45%)`	⬆️
...ages/base/sequence/BinarySequenceTransformer.scala	`100% <100%> (ø)`
...om/salesforce/op/utils/spark/OpSparkListener.scala	`98.7% <0%> (+1.29%)`	⬆️

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 b16b7cb...0238fe4. Read the comment docs.

leahmcguire · 2018-08-24T16:09:08Z

features/src/main/scala/com/salesforce/op/stages/base/sequence/BinarySequenceTransformer.scala

+    dataset.select(col("*"), functionUDF(struct(columns: _*)).as(getOutputFeatureName, meta))
+  }
+
+  private val transformNFn = FeatureSparkTypes.transform2N[I1, I2, O](transformFn)


private lazy val

leahmcguire · 2018-08-24T16:09:21Z

features/src/main/scala/com/salesforce/op/stages/base/sequence/BinarySequenceTransformer.scala

+  }
+
+  private val transformNFn = FeatureSparkTypes.transform2N[I1, I2, O](transformFn)
+  override def transformKeyValue: KeyValue => Any = {


I am not sure what should be lazy here. Please clarify.

@mikeloh77 you dont want to know 😛 but if you are curious, mainly because of SelectedModel class in ModelSelector

leahmcguire

Nice!

…ml/BinarySequenceEsimator

tovbinm

lgtm!!

mikeloh77 added 2 commits August 23, 2018 21:45

adding BinarySequenceEstimator and Transformer

d32c2da

fixing scalastyle

f46bef8

mikeloh77 requested a review from tovbinm August 24, 2018 05:06

mikeloh77 requested a review from leahmcguire as a code owner August 24, 2018 05:06

salesforce-cla bot added cla:missing cla:signed and removed cla:missing labels Aug 24, 2018

Merge branch 'master' into ml/BinarySequenceEsimator

0f59979

tovbinm reviewed Aug 24, 2018

View reviewed changes

leahmcguire reviewed Aug 24, 2018

View reviewed changes

tovbinm and others added 5 commits August 24, 2018 16:41

Merge branch 'master' into ml/BinarySequenceEsimator

9545386

Merge branch 'master' into ml/BinarySequenceEsimator

1f9b25e

addressing pr comments

ea74622

Merge remote-tracking branch 'origin/ml/BinarySequenceEsimator' into …

23f0c16

…ml/BinarySequenceEsimator

Merge branch 'master' into ml/BinarySequenceEsimator

0238fe4

tovbinm changed the title ~~Ml/binary sequence esimator~~ Binary sequence estimator Aug 28, 2018

tovbinm approved these changes Aug 28, 2018

View reviewed changes

tovbinm merged commit f9348de into master Aug 28, 2018

tovbinm deleted the ml/BinarySequenceEsimator branch August 28, 2018 06:04

ericwayman pushed a commit that referenced this pull request Feb 8, 2019

Ml/binary sequence esimator (#84)

47d30c3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binary sequence estimator #84

Binary sequence estimator #84

mikeloh77 commented Aug 24, 2018

salesforce-cla bot commented Aug 24, 2018

tovbinm Aug 24, 2018

tovbinm Aug 24, 2018

tovbinm Aug 24, 2018

tovbinm Aug 24, 2018

tovbinm Aug 24, 2018

tovbinm Aug 24, 2018

tovbinm Aug 24, 2018

tovbinm Aug 24, 2018

leahmcguire Aug 24, 2018

tovbinm Aug 24, 2018

tovbinm Aug 24, 2018

tovbinm Aug 24, 2018

tovbinm Aug 24, 2018

codecov bot commented Aug 24, 2018 •

edited

Loading

leahmcguire Aug 24, 2018

leahmcguire Aug 24, 2018

mikeloh77 Aug 28, 2018

tovbinm Aug 28, 2018

leahmcguire left a comment

tovbinm left a comment

Binary sequence estimator #84

Binary sequence estimator #84

Conversation

mikeloh77 commented Aug 24, 2018

salesforce-cla bot commented Aug 24, 2018

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

codecov bot commented Aug 24, 2018 • 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

leahmcguire left a comment

Choose a reason for hiding this comment

tovbinm left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 24, 2018 •

edited

Loading