Improved datetime unit transformer shortcuts #316

crupley · 2019-05-13T00:14:08Z

Related issues
Improved datetime unit transformer shortcuts #315

Describe the proposed solution
Enhanced RichDate and RichDateTime to allow conversion to various time periods using a common class. Each now supports a toTimePeriod method which can convert to any of the current TimePeriods:

TimePeriod.DayOfMonth
TimePeriod.DayOfWeek
TimePeriod.DayOfYear
TimePeriod.HourOfDay
TimePeriod.MonthOfYear
TimePeriod.WeekOfMonth
TimePeriod.WeekOfYear

Describe alternatives you've considered
There are many other potential values that could be extracted from the datetime, but I stuck to only those defined in the TimePeriod enum.

Additional context

codecov · 2019-05-13T00:30:19Z

Codecov Report

Merging #316 into master will increase coverage by 1.3%.
The diff coverage is 16.66%.

@@            Coverage Diff            @@
##           master     #316     +/-   ##
=========================================
+ Coverage   80.89%   82.19%   +1.3%     
=========================================
  Files         325      327      +2     
  Lines       10581    10593     +12     
  Branches      349      550    +201     
=========================================
+ Hits         8559     8707    +148     
+ Misses       2022     1886    -136

Impacted Files	Coverage Δ
...salesforce/op/stages/impl/feature/TimePeriod.scala	`0% <0%> (ø)`
...op/stages/impl/feature/TimePeriodTransformer.scala	`100% <100%> (ø)`
.../scala/com/salesforce/op/dsl/RichDateFeature.scala	`94.44% <50%> (-5.56%)`	⬇️
...alesforce/op/cli/gen/templates/SimpleProject.scala	`0% <0%> (-100%)`	⬇️
...om/salesforce/op/local/OpWorkflowRunnerLocal.scala	`0% <0%> (-100%)`	⬇️
.../scala/com/salesforce/op/cli/gen/ProblemKind.scala	`0% <0%> (-100%)`	⬇️
...cala/com/salesforce/op/cli/gen/FileInProject.scala	`0% <0%> (-100%)`	⬇️
...in/scala/com/salesforce/op/cli/CommandParser.scala	`0% <0%> (-98.12%)`	⬇️
...com/salesforce/op/local/OpWorkflowModelLocal.scala	`0% <0%> (-97.37%)`	⬇️
...cala/com/salesforce/op/cli/gen/ProblemSchema.scala	`0% <0%> (-96.56%)`	⬇️
... and 68 more

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 9d01538...00b38cb. Read the comment docs.

tovbinm · 2019-05-13T04:13:13Z

core/src/main/scala/com/salesforce/op/dsl/RichDateFeature.scala

+   * @return Integer feature of time period value
+   */
+  def toTimePeriod(period: TimePeriod): FeatureLike[Integral] = {
+    val periodFun: Long => Int = period match {


Lets add a concrete transformer class that extends UnaryTransformer instead of using UnaryLambdaTransformer

tovbinm · 2019-05-13T04:15:04Z

core/src/main/scala/com/salesforce/op/dsl/RichDateFeature.scala

+    )
+  }
+
+  def toDayOfMonth(): FeatureLike[Integral] = toTimePeriod(TimePeriod.DayOfMonth)


Is there much value of having these methods? let's just have def toTimePeriod(period: TimePeriod) and that way you wont need this abstract class bu simply add toTimePeriod to both RichDateTimeFeature and RichDateFeature.

I think so. As a user, I would much prefer to use these shortcuts and likely not use the time period shortcuts at all.

Once introduced, we would have to maintain them ;) and each time a new entry in enum is added we would need to add a new method. that's why I just prefer a single method with an enum argument.

tovbinm · 2019-05-13T04:15:50Z

core/src/test/scala/com/salesforce/op/dsl/RichDateFeatureTest.scala

+import org.scalatest.junit.JUnitRunner
+
+@RunWith(classOf[JUnitRunner])
+class RichDateFeatureTest extends FlatSpec with TestSparkContext {


once you have a concrete transformer, please use OpTransformerSpec to test it

… cr/timeperiod

tovbinm

some minor comments, otherwise lgtm

tovbinm · 2019-05-14T16:30:14Z

core/src/main/scala/com/salesforce/op/stages/impl/feature/TimePeriodTransformer.scala

@@ -0,0 +1,41 @@
+package com.salesforce.op.stages.impl.feature


please add license header

tovbinm · 2019-05-14T16:31:57Z

core/src/main/scala/com/salesforce/op/stages/impl/feature/TimePeriodTransformer.scala

+)(
+  implicit override val tti: TypeTag[I]
+) extends UnaryTransformer[I, Integral](operationName = "dateToTimePeriod", uid = uid){
+  val periodFun: Long => Int = period match {


just make this a regular function def periodConversion(time: Long): Int = period match { ... }

tovbinm · 2019-05-14T16:35:15Z

@crupley fyi, we also have DateList, DateTimeList, DateMap and DateTimeMap feature types. are you planning to add similar shortcuts to those in the following PRs?

crupley · 2019-05-14T17:05:11Z

Thanks for pointing that out; yes, I can add that in a new PR

gerashegalov · 2019-05-14T17:35:10Z

core/src/main/scala/com/salesforce/op/stages/impl/feature/TimePeriodTransformer.scala

+    case TimePeriod.HourOfDay => new JDateTime(t, DateTimeUtils.DefaultTimeZone).hourOfDay.get
+    case TimePeriod.MonthOfYear => new JDateTime(t, DateTimeUtils.DefaultTimeZone).monthOfYear.get
+    case TimePeriod.WeekOfMonth =>
+      val dt = new JDateTime(t, DateTimeUtils.DefaultTimeZone)


JDateTime(t, DateTimeUtils.DefaultTimeZone) is repeated on every case. Let us pull it before the pattern match.

gerashegalov · 2019-05-14T17:45:58Z

core/src/main/scala/com/salesforce/op/stages/impl/feature/TimePeriodTransformer.scala

+)(
+  implicit override val tti: TypeTag[I]
+) extends UnaryTransformer[I, Integral](operationName = "dateToTimePeriod", uid = uid){
+  def periodFun(t: Long): Int = period match {


would it make sense to use OOP and put

sealed abstract class TimePeriod extends EnumEntry with Serializable { def periodFun(t: Long): Int } case object Blah extends TimePeriod { override def periodFun(t: Long): Int = ??? }

and call period.periodFun here

put logic into enum? I am not a big fan of this pattern.

Good idea; let me try it

Eliminates the case matching or having to pass a trait around with the conversion function in it. What do you think of the implementation, @tovbinm?

gerashegalov · 2019-05-14T22:00:06Z

features/src/main/scala/com/salesforce/op/stages/impl/feature/TimePeriod.scala

-  case object MonthOfYear extends TimePeriod
-  case object WeekOfMonth extends TimePeriod
-  case object WeekOfYear extends TimePeriod
+  case object DayOfMonth extends TimePeriod { def extractFromTime(t: Long): Int = longToDateTime(t).dayOfMonth.get }


I think it's easier to read if the function definition is after a line break.

tovbinm · 2019-05-14T22:12:01Z

core/src/test/scala/com/salesforce/op/stages/impl/feature/TimePeriodTransformerTest.scala

+        case TimePeriod.MonthOfYear => Array(Integral(3), Integral(11), Integral(3), Integral.empty, Integral(4))
+        case TimePeriod.WeekOfMonth => Array(Integral(2), Integral(1), Integral(1), Integral.empty, Integral(4))
+        case TimePeriod.WeekOfYear => Array(Integral(11), Integral(45), Integral(10), Integral.empty, Integral(18))
+        case _ => throw new Exception(s"Unexpected TimePeriod encountered, $tp")


in tests instead of throw new Exception(s"Unexpected TimePeriod encountered, $tp") do fail(s"Unexpected TimePeriod encountered, $tp")

crupley added 7 commits April 30, 2019 13:05

add time period conversion shortcuts

8ee8410

move time period transformers into trait

b80302c

remove trait

668074d

add test

0ae7a3b

create common class and update test

b741d5f

restore line

d92ab72

remove unused import

ccd5291

crupley requested review from leahmcguire and tovbinm as code owners May 13, 2019 00:14

Merge remote-tracking branch 'upstream/master' into cr/timeperiod

02627ff

tovbinm reviewed May 13, 2019

View reviewed changes

tovbinm and others added 7 commits May 13, 2019 09:40

Merge branch 'master' into cr/timeperiod

56351da

Merge branch 'master' into cr/timeperiod

b785401

switch to dedicated unary transformer and new test

e43ce7e

use shortcut in test

76eb332

Merge remote-tracking branch 'upstream/master' into cr/timeperiod

86b8df0

Merge branch 'cr/timeperiod' of github.com:crupley/TransmogrifAI into…

6d12c9b

… cr/timeperiod

restore blank line

4bb13c5

tovbinm approved these changes May 14, 2019

View reviewed changes

crupley added 3 commits May 14, 2019 09:49

add header

5f91f14

convert periodFun to function

191cd15

add header

d777d17

gerashegalov reviewed May 14, 2019

View reviewed changes

crupley and others added 2 commits May 14, 2019 12:24

move periodFun into TimePeriod enum

7b92ed8

Merge branch 'master' into cr/timeperiod

00b38cb

gerashegalov reviewed May 14, 2019

View reviewed changes

tovbinm approved these changes May 14, 2019

View reviewed changes

tovbinm reviewed May 14, 2019

View reviewed changes

tovbinm merged commit 18a9243 into salesforce:master May 14, 2019

crupley deleted the cr/timeperiod branch May 14, 2019 22:58

crupley mentioned this pull request May 14, 2019

Improved datetime unit transformer shortcuts - Part 2 #319

Merged

This was referenced Jul 10, 2019

0.6.0 Release #360

Closed

0.6.0 release #364

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved datetime unit transformer shortcuts #316

Improved datetime unit transformer shortcuts #316

crupley commented May 13, 2019 •

edited

Loading

codecov bot commented May 13, 2019 •

edited

Loading

tovbinm May 13, 2019

tovbinm May 13, 2019

crupley May 13, 2019

tovbinm May 13, 2019

tovbinm May 13, 2019

tovbinm left a comment

tovbinm May 14, 2019

tovbinm May 14, 2019

tovbinm commented May 14, 2019

crupley commented May 14, 2019

gerashegalov May 14, 2019

gerashegalov May 14, 2019

tovbinm May 14, 2019

crupley May 14, 2019

crupley May 14, 2019

gerashegalov May 14, 2019

tovbinm May 14, 2019 •

edited

Loading

		@@ -0,0 +1,41 @@
		package com.salesforce.op.stages.impl.feature

Improved datetime unit transformer shortcuts #316

Improved datetime unit transformer shortcuts #316

Conversation

crupley commented May 13, 2019 • edited Loading

codecov bot commented May 13, 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

tovbinm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tovbinm commented May 14, 2019

crupley commented May 14, 2019

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

tovbinm May 14, 2019 • edited Loading

Choose a reason for hiding this comment

crupley commented May 13, 2019 •

edited

Loading

codecov bot commented May 13, 2019 •

edited

Loading

tovbinm May 14, 2019 •

edited

Loading