Update Datetime format inference #1666
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1666 +/- ##
=======================================
Coverage 98.79% 98.79%
=======================================
Files 98 98
Lines 11786 11800 +14
=======================================
+ Hits 11644 11658 +14
Misses 142 142
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
…into add_timezone_to_format
woodwork/utils.py
Outdated
| "%d/%m/%y", | ||
| "%y/%d/%m", | ||
| "%d/%m/%y %H:%M:%S", | ||
| "%y/%d/%m %H:%M:%S", | ||
| "%y/%d/%m %H:%M:%S%z", |
There was a problem hiding this comment.
This doesn't support timezone for all formats with two digit years, we'd want the timezone appended to the end of every iteration of two digit years. I'd recommend adding it as a secondary check if two digit year inference fails
| .astype("datetime64[ns]") | ||
| ) | ||
| df = pd.concat([head, pd.Series(dates), tail]).reset_index(drop=True) | ||
| df = pd.to_datetime(df, utc=True) |
There was a problem hiding this comment.
Is utc=True valid for the non-timezone use cases? I think this is converting all the datetimes to including timezones right? The tests are all passing so I think it's alright but I just wanted to check
There was a problem hiding this comment.
We ultimately convert everything to UTC anyway right? If we got something in a different timezone would this just convert it to UTC?
There was a problem hiding this comment.
Yeah I think it would do that
| "%y/%m/%d", | ||
| "%m/%d/%y %H:%M:%S", |
There was a problem hiding this comment.
How come these were removed?
There was a problem hiding this comment.
It's because I add on the timestamps below in a different way that makes sure every format is paired up with a timezone rather than only a few
This is the line:
time_stamp_formats = []
for format_ in datetime_only_formats:
time_stamp_formats.append(format_ + " %H:%M:%S")
time_stamp_formats_with_timezone = []
for format_ in datetime_only_formats:
time_stamp_formats_with_timezone.append(format_ + " %H:%M:%S%z")
|
Couple of questions and needs a rebase, but otherwise looking good |
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rstto include this pull request.