-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
[BUG] Make the datetime guesser cover edge case where the first date's formatting isn't the whole list's formatting #62965
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
base: main
Are you sure you want to change the base?
Conversation
| cdef void _maybe_warn_about_dayfirst(format: str, bint dayfirst) noexcept: | ||
| """Warn if guessed datetime format doesn't respect dayfirst argument.""" | ||
| cdef: | ||
| int day_index = format.find("%d") | ||
| int month_index = format.find("%m") | ||
|
|
||
| if (day_index != -1) and (month_index != -1): | ||
| if (day_index > month_index) and dayfirst: | ||
| warnings.warn( | ||
| f"Parsing dates in {format} format when dayfirst=True was specified. " | ||
| "Pass `dayfirst=False` or specify a format to silence this warning.", | ||
| UserWarning, | ||
| stacklevel=find_stack_level(), | ||
| ) | ||
| if (day_index < month_index) and not dayfirst: | ||
| warnings.warn( | ||
| f"Parsing dates in {format} format when dayfirst=False (the default) " | ||
| "was specified. " | ||
| "Pass `dayfirst=True` or specify a format to silence this warning.", | ||
| UserWarning, | ||
| stacklevel=find_stack_level(), | ||
| ) | ||
|
|
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.
I wasn't sure what to do with this code. Are these kinds of warnings helpful? They were causing my tests to fail on pytest teardown, and was being logged on tests which had different formatting for the input string (So if I specified dayfirst = True on a month first input it'd raise the warning, and if I specified dayfirst=False on a day first test case it'd fail.
Happy to revert this or make some other change
| ) -> str | None: | ||
| # Try to guess the format based on the first non-NaN element, return None if can't | ||
| if (first_non_null := tslib.first_non_null(arr)) != -1: | ||
| if type(first_non_nan_element := arr[first_non_null]) is str: |
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.
I wanted to replace if type(x) == str with isinstance(x, str) so that we could consume np.str types
|
cc @MarcoGorelli i think this was your idea |
… good before fixing everything
|
Let me know if you like the changes (from a high level) and I can fix the rest of the test cases and tag you when things are ready. |
This PR fixes some behavior with the datetime guesser code. Previously, if it was supplied with an array such as:
Then the parser would call
tslib.first_non_nulland assume that the format string for the entire array was month/day/year while the series is actually day/month/year.doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.