Skip to content
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

BUG/ENH: Handle AmbiguousTimeError in date rounding #22647

Merged
merged 9 commits into from Sep 23, 2018

Conversation

Projects
None yet
3 participants
@mroeschke
Copy link
Member

commented Sep 9, 2018

  • closes #18946
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

Discussed in #22560, adding an ambiguous argument to round, ceil and floor so the user can dictate how to handle rounding timestamps that land on ambiguous times.

@pep8speaks

This comment has been minimized.

Copy link

commented Sep 9, 2018

Hello @mroeschke! Thanks for updating the PR.

Comment last updated on September 19, 2018 at 20:20 Hours UTC
@codecov

This comment has been minimized.

Copy link

commented Sep 9, 2018

Codecov Report

Merging #22647 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #22647      +/-   ##
==========================================
+ Coverage   92.17%   92.17%   +<.01%     
==========================================
  Files         169      169              
  Lines       50769    50772       +3     
==========================================
+ Hits        46798    46801       +3     
  Misses       3971     3971
Flag Coverage Δ
#multiple 90.59% <100%> (ø) ⬆️
#single 42.33% <55.55%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/indexes/datetimelike.py 98.1% <100%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f87fe14...ba7eddd. Read the comment docs.

@mroeschke mroeschke added this to the 0.24.0 milestone Sep 11, 2018

@jreback
Copy link
Contributor

left a comment

looks fine

ambiguous : bool, 'NaT', default 'raise'
- bool contains flags to determine if time is dst or not (note
that this flag is only applicable for ambiguous fall dst dates)
- 'NaT' will return NaT for an ambiguous time

This comment has been minimized.

Copy link
@jreback

jreback Sep 18, 2018

Contributor

can you add versionadded tags here (and other new arg places)

that this flag is only applicable for ambiguous fall dst dates)
- 'NaT' will return NaT for an ambiguous time
- 'raise' will raise an AmbiguousTimeError for an ambiguous time

This comment has been minimized.

Copy link
@jreback

jreback Sep 18, 2018

Contributor

may make sense to provide a template for the doc-strings (can be followup)

- 'infer' will attempt to infer fall dst-transition hours based on
order
- bool-ndarray where True signifies a DST time, False designates
a non-DST time (note that this flag is only applicable for

This comment has been minimized.

Copy link
@jreback

jreback Sep 18, 2018

Contributor

versionadded

can you test for .dt accessors as well?

This comment has been minimized.

Copy link
@mroeschke

mroeschke Sep 19, 2018

Author Member

The .dt accessor tests are here:

def test_dt_round(self, method, dates):

@mroeschke

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2018

Addressed your comments and all green @jreback

@jreback jreback merged commit f67b90d into pandas-dev:master Sep 23, 2018

6 checks passed

ci/circleci: py27_compat Your tests passed on CircleCI!
Details
ci/circleci: py35_ascii Your tests passed on CircleCI!
Details
ci/circleci: py36_locale Your tests passed on CircleCI!
Details
ci/circleci: py36_locale_slow Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pandas-dev.pandas Build #50 succeeded
Details
@jreback

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2018

thanks @mroeschke

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.