search

salesforce / TransmogrifAI

TransmogrifAI (pronounced trăns-mŏgˈrə-fī) is an AutoML library for building modular, reusable, strongly typed machine learning workflows on Apache Spark with minimal hand-tuning
https://transmogrif.ai
BSD 3-Clause "New" or "Revised" License
2.24k stars 393 forks source link

Improve WeekOfMonth in date transformers #323

Closed gerashegalov closed 5 years ago

gerashegalov commented 5 years ago

Related issues Fixes #322

Describe the proposed solution Use an approach from https://stackoverflow.com/questions/3806473/python-week-number-of-the-month. Reuse logic throughout datetime transformers.

codecov[bot] commented 5 years ago

Codecov Report

Merging #323 into master will increase coverage by 0.03%. The diff coverage is 70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #323      +/-   ##
==========================================
+ Coverage   86.46%   86.49%   +0.03%     
==========================================
  Files         329      329              
  Lines       10608    10598      -10     
  Branches      346      336      -10     
==========================================
- Hits         9172     9167       -5     
+ Misses       1436     1431       -5
Impacted Files Coverage Δ
...tages/impl/feature/TimePeriodListTransformer.scala 100% <100%> (ø) :arrow_up:
...stages/impl/feature/TimePeriodMapTransformer.scala 100% <100%> (ø) :arrow_up:
...op/stages/impl/feature/TimePeriodTransformer.scala 100% <100%> (ø) :arrow_up:
...ges/impl/feature/DateToUnitCircleTransformer.scala 100% <100%> (ø) :arrow_up:
...salesforce/op/stages/impl/feature/TimePeriod.scala 25% <25%> (+25%) :arrow_up:
...es/src/main/scala/com/salesforce/op/OpParams.scala 85.71% <0%> (-4.09%) :arrow_down:
.../op/features/types/FeatureTypeSparkConverter.scala 99.11% <0%> (+0.88%) :arrow_up:

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 689b71f...db51d62. Read the comment docs.

tovbinm commented 5 years ago

Looks a bit of hack to me. Why cant we use standard library and avoid custom computations?

shaeselix commented 5 years ago

Could we use something from here? https://docs.oracle.com/javase/8/docs/api/java/time/temporal/WeekFields.html

gerashegalov commented 5 years ago

Looks a bit of hack to me. Why cant we use standard library and avoid custom computations?

@tovbinm we currently rely on joda that does not provide this functionality. We should switch to JDK8+ java.time as @shaeselix suggests when we replace joda in the context of #287

tovbinm commented 5 years ago

Great. Let’s do that instead.

gerashegalov commented 5 years ago

@tovbinm @shaeselix please take a look again when you can

gerashegalov commented 5 years ago

we have more usages of joda. I'd prefer to tackle it area by area. this PR already grew from a simple fix to larger than I anticipated.