Skip to content

Update DateToHoliday and DistanceToHoliday primitives to work with timezone-aware inputs#2056

Merged
gsheni merged 3 commits into
mainfrom
tz-aware-prim-fix
May 11, 2022
Merged

Update DateToHoliday and DistanceToHoliday primitives to work with timezone-aware inputs#2056
gsheni merged 3 commits into
mainfrom
tz-aware-prim-fix

Conversation

@thehomebrewnerd

Copy link
Copy Markdown
Contributor

This PR updates the DateToHoliday and DistanceToHoliday primitives to work with timezone-aware inputs and adds new tests to verify the correct answer is returned.

@codecov

codecov Bot commented May 11, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2056 (c40a281) into main (e762d77) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2056   +/-   ##
=======================================
  Coverage   99.20%   99.20%           
=======================================
  Files         143      143           
  Lines       16540    16554   +14     
=======================================
+ Hits        16408    16422   +14     
  Misses        132      132           
Impacted Files Coverage Δ
...imitives/standard/datetime_transform_primitives.py 100.00% <100.00%> (ø)
...ts/primitive_tests/test_datetoholiday_primitive.py 100.00% <100.00%> (ø)
...rimitive_tests/test_distancetoholiday_primitive.py 100.00% <100.00%> (ø)

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 e762d77...c40a281. Read the comment docs.

df = df.dropna()
df = df.sort_values("date")
df.date = df.date.dt.normalize()
df.date = df.date.dt.normalize().dt.date.astype("datetime64[ns]")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to normalize if we are also extracting the date?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the normalize calls aren't necessary anymore. Removed. Also updated to (I think) improve readability of these lines a little bit.

),
"non_timezone_aware_no_time": pd.date_range("2018-07-03", periods=3),
"timezone_aware_with_time": pd.date_range(
"2018-07-03 09:00", periods=3

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we test with times where if converted to UTC, some times would change dates?

@rwedge rwedge left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@gsheni gsheni merged commit 13ecb6d into main May 11, 2022
@gsheni gsheni deleted the tz-aware-prim-fix branch May 11, 2022 18:45
@thehomebrewnerd thehomebrewnerd mentioned this pull request May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants