-
Notifications
You must be signed in to change notification settings - Fork 393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved datetime unit transformer shortcuts - Part 2 #319
Conversation
Codecov Report
@@ Coverage Diff @@
## master #319 +/- ##
==========================================
- Coverage 86.44% 82.61% -3.83%
==========================================
Files 327 329 +2
Lines 10602 10608 +6
Branches 336 533 +197
==========================================
- Hits 9165 8764 -401
- Misses 1437 1844 +407
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, see some minor comments
uid: String = UID[TimePeriodTransformer[_]] | ||
)( | ||
implicit override val tti: TypeTag[I] | ||
) extends UnaryTransformer[I, OPVector](operationName = "dateToTimePeriod", uid = uid) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use a different and distinctive operation name, say dateListToTimePeriod
uid: String = UID[TimePeriodTransformer[_]] | ||
)( | ||
implicit override val tti: TypeTag[I] | ||
) extends UnaryTransformer[I, IntegralMap](operationName = "dateToTimePeriod", uid = uid) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use a different and distinctive operation name, say dateMapToTimePeriod
) extends UnaryTransformer[I, OPVector](operationName = "dateToTimePeriod", uid = uid) { | ||
|
||
override def transformFn: I => OPVector = | ||
(i: I) => i.value.map(t => period.extractFromTime(t).toDouble).toVector.toOPVector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why i.value.map(t => period.extractFromTime(t).toDouble).toOPVector
does not work?
class TimePeriodListTransformer[I <: DateList] | ||
( | ||
val period: TimePeriod, | ||
uid: String = UID[TimePeriodTransformer[_]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix uid - UID[TimePeriodListTransformer[_]]
class TimePeriodMapTransformer[I <: DateMap] | ||
( | ||
val period: TimePeriod, | ||
uid: String = UID[TimePeriodTransformer[_]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here - fix uid - UID[TimePeriodMapTransformer[_]]
Related issues
Improved datetime unit transformer shortcuts #315
Part 1: #316
Describe the proposed solution
Add additional time period support for
map
andlist
date/datetime feature types.TimePeriodMapTransformer
TimePeriodListTransformer
toTimePeriod
shortcuts inRich-{DateList/DateTimeList/DateMap/DateTimeMap}
Describe alternatives you've considered
None
Additional context
None