Skip to content

move ndarray-returning functions to EA mixin classes #21722

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 4 commits into from
Jul 5, 2018

Conversation

jbrockmendel
Copy link
Member

Orthogonal to other EAMixin PRs.

The methods this moves are unchanged. Docstrings changed e.g. "DatetimeIndex" to "Datetime Array/Index". I'm open to suggestions for better wording.

The error message in PeriodArrayMixin._maybe_convert_timedelta is changed to use type(self).__name__ instead of hardcoding PeriodIndex. Ditto the warning issued by PeriodArrayMixin.freq.setter.

@pep8speaks
Copy link

pep8speaks commented Jul 4, 2018

Hello @jbrockmendel! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on July 04, 2018 at 17:06 Hours UTC

@codecov
Copy link

codecov bot commented Jul 4, 2018

Codecov Report

Merging #21722 into master will increase coverage by <.01%.
The diff coverage is 87.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #21722      +/-   ##
==========================================
+ Coverage   91.92%   91.93%   +<.01%     
==========================================
  Files         158      158              
  Lines       49702    49716      +14     
==========================================
+ Hits        45690    45704      +14     
  Misses       4012     4012
Flag Coverage Δ
#multiple 90.3% <87.09%> (ø) ⬆️
#single 42.01% <38.7%> (+0.01%) ⬆️
Impacted Files Coverage Δ
pandas/core/indexes/datetimes.py 95.48% <ø> (-0.01%) ⬇️
pandas/core/arrays/datetimes.py 100% <100%> (ø) ⬆️
pandas/core/arrays/timedelta.py 100% <100%> (ø) ⬆️
pandas/core/indexes/period.py 93.35% <100%> (+0.81%) ⬆️
pandas/core/indexes/datetimelike.py 96.98% <100%> (+0.11%) ⬆️
pandas/core/indexes/timedeltas.py 91.05% <100%> (-0.04%) ⬇️
pandas/core/arrays/period.py 86.53% <80.55%> (-13.47%) ⬇️
pandas/core/arrays/datetimelike.py 94.38% <94.11%> (-0.14%) ⬇️
pandas/core/reshape/tile.py 94.73% <0%> (ø) ⬆️
pandas/plotting/_core.py 83.57% <0%> (+0.03%) ⬆️

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 2b13605...1a3fdef. Read the comment docs.

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.

looks good. doc comments

# ------------------------------------------------------------------
# Arithmetic Methods

def _sub_datelike(self, other):
assert other is not NaT
return NotImplemented

def _maybe_convert_timedelta(self, other):
if isinstance(
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a doc-string to this one as it accepts many types

@jreback jreback added ExtensionArray Extending pandas with custom dtypes or arrays. Datetime Datetime data dtype labels Jul 4, 2018
@jreback jreback added this to the 0.24.0 milestone Jul 4, 2018
@jreback
Copy link
Contributor

jreback commented Jul 4, 2018

lgtm. ping on green.

@jbrockmendel
Copy link
Member Author

ping

@jreback jreback merged commit 1c5b342 into pandas-dev:master Jul 5, 2018
@jreback
Copy link
Contributor

jreback commented Jul 5, 2018

thanks!

@jbrockmendel jbrockmendel deleted the dtarrays-funcs branch July 6, 2018 14:36
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype ExtensionArray Extending pandas with custom dtypes or arrays.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants