BUG #34621 added nanosecond support to class Period #34720
Conversation
|
See #34621 (comment). I don't think this is a bug. Edit: Misunderstood, confirmed a bug |
|
Hello @OlivierLuG! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-06-16 18:43:06 UTC |
pandas/_libs/tslibs/period.pyx
Outdated
| nanosecond = ts.nanosecond | ||
| if nanosecond != 0: | ||
| reso = 'nanosecond' | ||
| except (ValueError, OutOfBoundsDatetime): |
There was a problem hiding this comment.
OOBDatetime is a subclass of ValueError so this is uneeded
There was a problem hiding this comment.
I've first try to only use OutOfBoundsDatetime but the test code fails on many VM. Here is an example: https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=37155&view=logs&j=a3a13ea8-7cf0-5bdb-71bb-6ac8830ae35c&t=add65f64-6c25-5783-8fd6-d9aa1b63d9d4&l=471
pandas/_libs/tslibs/period.pyx:2410: in pandas._libs.tslibs.period.Period.__new__
ts = Timestamp(value, freq=freq)
pandas/_libs/tslibs/timestamps.pyx:848: in pandas._libs.tslibs.timestamps.Timestamp.__new__
ts = convert_to_tsobject(ts_input, tz, unit, 0, 0, nanosecond or 0)
pandas/_libs/tslibs/conversion.pyx:333: in pandas._libs.tslibs.conversion.convert_to_tsobject
return convert_str_to_tsobject(ts, tz, unit, dayfirst, yearfirst)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
> raise ValueError("could not convert string to Timestamp")
E ValueError: could not convert string to Timestamp
pandas/_libs/tslibs/conversion.pyx:581: ValueError
There was a problem hiding this comment.
@OlivierLuG I think Jeff meant that it should be enough to only use ValueError, as that would already cover the OutOfBoundsDatetime case
There was a problem hiding this comment.
OK, I got it. I will try a new PR with only ValueError
pandas/_libs/tslibs/period.pyx
Outdated
| @@ -2404,6 2406,13 @@ class Period(_Period): | |||
| value = str(value) | |||
| value = value.upper() | |||
| dt, reso = parse_time_string(value, freq) | |||
| try: | |||
| ts = Timestamp(value, freq=freq) | |||
There was a problem hiding this comment.
A new PR is ready (waiting for the question about else)
| @@ -484,6 484,22 @@ def test_period_cons_combined(self): | |||
| with pytest.raises(ValueError, match=msg): | |||
| Period("2011-01", freq="1D1W") | |||
|
|
|||
| @pytest.mark.parametrize("day_", ["1970/01/01 ", "2020-12-31 ", "1981/09/13 "]) | |||
There was a problem hiding this comment.
i dont think the trailing underscores are necessary for these names
| @@ -2404,6 2405,14 @@ class Period(_Period): | |||
| value = str(value) | |||
| value = value.upper() | |||
| dt, reso = parse_time_string(value, freq) | |||
| try: | |||
| ts = Timestamp(value) | |||
| except ValueError: | |||
There was a problem hiding this comment.
we end up parsing this twice which is not good, yeah i think this needs to be integrated a bit more to parse_time_string.
i guess the currently soln would be ok in the interim, but would need to run asv's on all periods to see what kind of perf hit here.
There was a problem hiding this comment.
As far I read, nanosecond was not accepted into dateutil module. This interim solution could stand for a while.
I stay tuned...
There was a problem hiding this comment.
As far I read, nanosecond was not accepted into dateutil module. This interim solution could stand for a while.
I stay tuned...
we don't wait for dateutil in this, as it will not likey every support nanosecond because the standard library does not
There was a problem hiding this comment.
@OlivierLuG is this still active? If yes can you merge master & resolve conflicts and address comments
| @@ -2404,6 2405,14 @@ class Period(_Period): | |||
| value = str(value) | |||
| value = value.upper() | |||
| dt, reso = parse_time_string(value, freq) | |||
| try: | |||
| ts = Timestamp(value) | |||
| except ValueError: | |||
There was a problem hiding this comment.
As far I read, nanosecond was not accepted into dateutil module. This interim solution could stand for a while.
I stay tuned...
we don't wait for dateutil in this, as it will not likey every support nanosecond because the standard library does not
|
this looked ok, pls merge master and address comments, ping on green |
|
Closing in favor of #38175 |
Closes #34621
Closes #17053
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffThe class Period do not support nanosecond. I've made a quick and dirty code to support nanoseconds. I have struggled to find an alternative to dateutil.parser, but I didn't found an alternative.
The PR may be a performance issue as I've add a Timestamp constructor to the dateutil.parser. There is for sure a better solution. Please let me know !