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

REF: Refactor Date/TimeLikeOps #24038

Merged
merged 2 commits into from
Dec 2, 2018

Conversation

TomAugspurger
Copy link
Contributor

@TomAugspurger TomAugspurger commented Dec 1, 2018

No real functional changes, just an inheritance reorganization to make the diff at #24024 smaller.

Changes:

  • Removes DatelikeOps from PeriodIndex (already had strftime via delegation)
  • Moves Date/TimelikeOps from DatetimeIndex to DatetimeArray
  • Moves TimelikeOps from TimedeltaIndex to TimedeltaArray

Moves the implementation / inheritence from indexes to arrays.
@TomAugspurger TomAugspurger added the Refactor Internal refactoring of code label Dec 1, 2018
@pep8speaks
Copy link

Hello @TomAugspurger! Thanks for submitting the PR.

@TomAugspurger TomAugspurger added the Datetime Datetime data dtype label Dec 1, 2018
@TomAugspurger TomAugspurger added this to the 0.24.0 milestone Dec 1, 2018
@TomAugspurger TomAugspurger mentioned this pull request Dec 1, 2018
12 tasks
@codecov
Copy link

codecov bot commented Dec 1, 2018

Codecov Report

Merging #24038 into master will decrease coverage by <.01%.
The diff coverage is 48.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24038      +/-   ##
==========================================
- Coverage   42.46%   42.45%   -0.01%     
==========================================
  Files         161      161              
  Lines       51557    51557              
==========================================
- Hits        21892    21889       -3     
- Misses      29665    29668       +3
Flag Coverage Δ
#single 42.45% <48.07%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/period.py 39.82% <100%> (ø) ⬆️
pandas/core/indexes/datetimelike.py 53.54% <100%> (+1%) ⬆️
pandas/core/arrays/timedeltas.py 50.28% <100%> (ø) ⬆️
pandas/core/indexes/datetimes.py 52.18% <100%> (ø) ⬆️
pandas/core/arrays/datetimes.py 63.48% <100%> (ø) ⬆️
pandas/core/indexes/timedeltas.py 39.82% <100%> (ø) ⬆️
pandas/core/arrays/datetimelike.py 50.53% <41.3%> (-1.25%) ⬇️

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 5b0610b...becf7d7. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 1, 2018

Codecov Report

Merging #24038 into master will decrease coverage by <.01%.
The diff coverage is 47.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24038      +/-   ##
==========================================
- Coverage   42.46%   42.45%   -0.01%     
==========================================
  Files         161      161              
  Lines       51557    51556       -1     
==========================================
- Hits        21892    21888       -4     
- Misses      29665    29668       +3
Flag Coverage Δ
#single 42.45% <47.05%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/period.py 39.82% <100%> (ø) ⬆️
pandas/core/indexes/datetimelike.py 53.54% <100%> (+1%) ⬆️
pandas/core/arrays/timedeltas.py 50.28% <100%> (ø) ⬆️
pandas/core/indexes/datetimes.py 52.18% <100%> (ø) ⬆️
pandas/core/arrays/datetimes.py 63.48% <100%> (ø) ⬆️
pandas/core/indexes/timedeltas.py 39.82% <100%> (ø) ⬆️
pandas/core/arrays/datetimelike.py 50.43% <40%> (-1.36%) ⬇️

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 5b0610b...54042f9. Read the comment docs.

@@ -80,6 +82,189 @@ def _get_attributes_dict(self):
return {k: getattr(self, k, None) for k in self._attributes}


class DatelikeOps(object):
"""
Common ops for DatetimeIndex/PeriodIndex, but not TimedeltaIndex.
Copy link
Member

Choose a reason for hiding this comment

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

Index --> Array/Index

from pandas import Index
return Index(self.format(date_format=date_format),
dtype=compat.text_type)
strftime.__doc__ = """
Copy link
Member

Choose a reason for hiding this comment

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

Not a big deal, but is there a reason not to put this docstring in the normal docstring place and then accomplish the formatting with @Substitution(...)?

Common ops for TimedeltaIndex/DatetimeIndex, but not PeriodIndex.
"""

_round_doc = (
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid having these in the TimedeltaArray/Index namespace?


Returns
-------
DatetimeIndex, TimedeltaIndex, or Series
Copy link
Member

Choose a reason for hiding this comment

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

Array/Index/Series

if 'tz' in attribs:
attribs['tz'] = None
return self._ensure_localized(
self._shallow_copy(result, **attribs), ambiguous, nonexistent
Copy link
Member

Choose a reason for hiding this comment

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

The array classes don't have _shallow_copy anymore. Use _simple_new directly?

-------
i8 1d array
"""
from pandas import Index
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way to avoid this? We've pretty assiduously kept the EA subclasses Index-ignorant so far

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what the old version did. I haven't looked at which is more appropriate here.

attribs['freq'] = None
if 'tz' in attribs:
attribs['tz'] = None
return self._ensure_localized(
Copy link
Member

Choose a reason for hiding this comment

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

Do the array classes have _ensure_localized?

@jbrockmendel
Copy link
Member

+1 on the scope. Needs tests for the methods added to DTA/TDA/PA

@TomAugspurger
Copy link
Contributor Author

TomAugspurger commented Dec 2, 2018

In response to
#24038 (review),
#24038 (review), and
#24038 (review), nothing has changed here. These classes end up mixed into exactly the same set as before. This is merely to keep the diff at #24024 smaller.

So in response to

Needs tests for the methods added to DTA/TDA/PA

That can't be done yet (at least I don't see an easy way), until the indexes have been split from the arrays. Things like _ensure_localized aren't on the array classes yet. I do that in #24024.

@jreback
Copy link
Contributor

jreback commented Dec 2, 2018

ok with going ahead with this to avoid blocking and making diffs easier. @jbrockmendel

@jbrockmendel
Copy link
Member

OK. I'll see if I can implement tests in a follow-up.

@jreback jreback merged commit 18b68c7 into pandas-dev:master Dec 2, 2018
@jreback
Copy link
Contributor

jreback commented Dec 2, 2018

thanks @TomAugspurger

@TomAugspurger TomAugspurger deleted the datelike-ops-split branch December 3, 2018 12:29
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype Refactor Internal refactoring of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants