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

DEPR: deprecate Timedelta.resolution #26839

Merged
merged 2 commits into from
Jun 17, 2019
Merged

Conversation

jbrockmendel
Copy link
Member

AFAICT we don't have any tests specifically for Timedelta.resolution, so added one for reso_str.

@WillAyd WillAyd added the Deprecate Functionality to remove in pandas label Jun 14, 2019
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.

do we have tests for datetime.resolution?

@@ -950,7 +950,7 @@ cdef class _Timedelta(timedelta):
return np.int64(self.value).view('m8[ns]')

@property
def resolution(self):
def reso_str(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

where did this name come from? maybe min_resolution_string? or just resolution_string is more informative

Copy link
Member Author

Choose a reason for hiding this comment

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

More or less chosen out of thin air. I think we might use it in some of the internal functions. resolution_string sounds better, will update

@jreback
Copy link
Contributor

jreback commented Jun 14, 2019

can you add a this in the deprecation issue

@jbrockmendel
Copy link
Member Author

do we have tests for datetime.resolution?

Doubtful. Timestamp.resolution and NaT.resolution are both wrong in subtly different ways, planning to do in separate PR.

@jbrockmendel
Copy link
Member Author

can you add a this in the deprecation issue

will do

@codecov
Copy link

codecov bot commented Jun 16, 2019

Codecov Report

Merging #26839 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26839      +/-   ##
==========================================
- Coverage   91.86%   91.85%   -0.01%     
==========================================
  Files         179      179              
  Lines       50700    50700              
==========================================
- Hits        46576    46571       -5     
- Misses       4124     4129       +5
Flag Coverage Δ
#multiple 90.45% <100%> (ø) ⬆️
#single 41.1% <0%> (-0.08%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/timedeltas.py 90.96% <100%> (ø) ⬆️
pandas/io/gbq.py 88.88% <0%> (-11.12%) ⬇️
pandas/core/frame.py 96.88% <0%> (-0.12%) ⬇️
pandas/util/testing.py 90.84% <0%> (-0.11%) ⬇️

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 2d2606d...9a67940. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 16, 2019

Codecov Report

Merging #26839 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26839      +/-   ##
==========================================
- Coverage   91.86%   91.86%   -0.01%     
==========================================
  Files         179      180       +1     
  Lines       50700    50710      +10     
==========================================
+ Hits        46576    46583       +7     
- Misses       4124     4127       +3
Flag Coverage Δ
#multiple 90.45% <100%> (ø) ⬆️
#single 41.1% <0%> (-0.08%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/timedeltas.py 90.96% <100%> (ø) ⬆️
pandas/io/gbq.py 88.88% <0%> (-11.12%) ⬇️
pandas/io/formats/printing.py 85.56% <0%> (-0.54%) ⬇️
pandas/core/frame.py 96.88% <0%> (-0.12%) ⬇️
pandas/util/testing.py 90.84% <0%> (-0.11%) ⬇️
pandas/core/arrays/datetimelike.py 97.92% <0%> (-0.01%) ⬇️
pandas/core/internals/managers.py 95.21% <0%> (-0.01%) ⬇️
pandas/core/internals/blocks.py 94.38% <0%> (-0.01%) ⬇️
pandas/core/arrays/datetimes.py 97.79% <0%> (-0.01%) ⬇️
pandas/core/indexes/interval.py 96.44% <0%> (ø) ⬆️
... and 20 more

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 2d2606d...9a67940. Read the comment docs.

@jreback jreback added this to the 0.25.0 milestone Jun 17, 2019
@jreback jreback merged commit baa77c3 into pandas-dev:master Jun 17, 2019
@jreback
Copy link
Contributor

jreback commented Jun 17, 2019

thanks @jbrockmendel

@jorisvandenbossche
Copy link
Member

We also have a Timestamp.resolution, DatetimeIndex.resolution, TimedeltaIndex.resolution. Should any of those be changed accordingly?

@jbrockmendel
Copy link
Member Author

We also have a Timestamp.resolution, DatetimeIndex.resolution, TimedeltaIndex.resolution. Should any of those be changed accordingly?

Yes. Also NaT.resolution. I'll open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pd.Timedelta.resolution is different from datetime.timedelta.resolution
4 participants