BUG: when flooring, ambiguous parameter unnecessarily used (and raising Error) #50024
Conversation
…iguos parameter is not one of the 4 correct options
| @td.skip_if_windows | ||
| def test_tz_convert_utc_with_system_utc(self): | ||
|
|
There was a problem hiding this comment.
Ah, i forgot to ask about this, I deleted it temporarily to run tests because using the @td.skip_if_windows decorator gives me an error I couldn't figure out
There was a problem hiding this comment.
can you put this back and add a whatsnew note? rest looks fine
pandas/_libs/tslibs/timestamps.pyx
Outdated
| raise ValueError("Cannot infer offset with only one time.") | ||
| if not isinstance(ambiguous, bool) and ambiguous not in {"NaT", "raise"}: | ||
| raise ValueError( | ||
| "ambiguous parameter must be one of: " |
There was a problem hiding this comment.
let's quote the parameter name, else it'll be ambiguous (😄 )
"'ambiguous' parameter must be one of: "
MarcoGorelli
changed the title
| # ------------------------------------------------------------------ | ||
| # Timestamp.__init__ with tz str or tzinfo | ||
|
|
There was a problem hiding this comment.
can you check over the file again please? let's not make irrelevant changes
doc/source/whatsnew/v1.5.3.rst
Outdated
| @@ -25,20 +25,21 @@ Fixed regressions | |||
| Bug fixes | |||
| ~~~~~~~~~ | |||
| - Bug in :meth:`.Styler.to_excel` leading to error when unrecognized ``border-style`` (e.g. ``"hair"``) provided to Excel writers (:issue:`48649`) | |||
| - | |||
| - Bug in :meth:`.Timestamp.floor` fixed tz_localize so 'ambiguous' parameter can only be True, False, 'NaT' or 'raise' and all other invalid arguments to ambiguous have same error, including 'infer' (:issue:`49565`) | |||
There was a problem hiding this comment.
this should go in v2.0.0.rst
as for the test, how about
Better error message when passing invalid values to ``ambiguous`` parameter in :meth:`Timestamp.tz_localize` (:issue:`49565`)
doc/source/whatsnew/v1.5.3.rst
Outdated
|
|
||
| .. --------------------------------------------------------------------------- | ||
| .. _whatsnew_153.other: | ||
|
|
||
| Other | ||
| ~~~~~ | ||
| - | ||
| - Modified test cases in test_timezones.py to reflect new error message from bug fix for bug: (:issue:`49565`) |
There was a problem hiding this comment.
this is unnecessary, let's remove it
doc/source/whatsnew/v1.5.3.rst
Outdated
| - | ||
|
|
||
| .. --------------------------------------------------------------------------- | ||
| .. _whatsnew_153.contributors: | ||
|
|
||
| Contributors | ||
| ~~~~~~~~~~~~ | ||
| - Julia Chaker Aoun |
There was a problem hiding this comment.
no need, this is populated automatically
There was a problem hiding this comment.
Hi! If you could approve or merge my PR by tomorrow I would really appreciate it! I'm working on this for a project and I get extra credit if the PR is approved :)
There was a problem hiding this comment.
you'll need to address the review comments first
There was a problem hiding this comment.
I believe I resolved the remaining comments in my last commit
There was a problem hiding this comment.
Looks good to me, I think this error message is clearer than the original (which kind of makes it look like pandas was trying to use the ambiguous parameter, but failed)
Over to @mroeschke in case there's further comments / objections
|
Thanks for approving the PR!!! This is the first time I contribute to an open-source project and it was really fun. I really appreciate your help :) |
|
Thanks @jcaoun |
flooring,ambiguousparameter unnecessarily used (and raising Error) #49565doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.