Skip to content

ENH: support nanoseconds in Period constructor #38175

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

Merged
merged 13 commits into from
Dec 2, 2020

Conversation

arw2019
Copy link
Member

@arw2019 arw2019 commented Nov 30, 2020

Picking up #34720

ASVs:

asv continuous -f 1.1 upstream/master HEAD -b ^period

       before           after         ratio
     [3f510600]       [2b2f1d04]
     <GH34621-nanos-for-Period^2>       <GH34621-nanos-for-Period>
+      7.46±0.2ms       11.6±0.1ms     1.55  period.PeriodIndexConstructor.time_from_ints('D', False)
+      7.57±0.3ms      11.4±0.03ms     1.51  period.PeriodIndexConstructor.time_from_ints('D', True)
+      52.8±0.6ms       59.2±0.4ms     1.12  period.PeriodIndexConstructor.time_from_ints_daily('D', True)
+      52.9±0.9ms       59.0±0.1ms     1.11  period.PeriodIndexConstructor.time_from_ints_daily('D', False)
-      3.21±0.2μs      2.90±0.02μs     0.90  period.Indexing.time_get_loc

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE DECREASED. 

cc @jreback #34720 (comment) in case we want this for 1.2

@jreback jreback added this to the 1.2 milestone Dec 2, 2020
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

lgtm. can you add a whatsnew in bug fixes for 1.2

cc @jbrockmendel

@jreback jreback merged commit efbcd68 into pandas-dev:master Dec 2, 2020
@jreback
Copy link
Contributor

jreback commented Dec 2, 2020

thanks @arw2019

Comment on lines 2397 to +2399
dt, reso = parse_time_string(value, freq)
try:
ts = Timestamp(value)
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we effectively doing the parsing twice, this way? (once in parse_time_string, once in Timestamp) Any way to avoid this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - @jreback suggested integrating this a bit more into parse_time_string

I opened #38312 to track resolution (I'll have a go at it if no one else does)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Period Period data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Period constructor does not take into account nanonseconds BUG: Period with nanoseconds (?)
3 participants