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/TST/REF: Datetimelike Arithmetic Methods #23215

Merged
merged 49 commits into from Oct 28, 2018

Conversation

Projects
None yet
5 participants
@jbrockmendel
Copy link
Member

commented Oct 17, 2018

@TomAugspurger @jorisvandenbossche @jreback as discussed

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry
attribs = self._get_attributes_dict()
if not is_period_dtype(self):
attribs['freq'] = None
return self._simple_new(result, **attribs)

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Oct 17, 2018

Author Member

This is only used once outside of tests. Much less verbose to inline.

This comment has been minimized.

Copy link
@jreback

jreback Oct 19, 2018

Contributor

hah I just made a comment on PeriodArray about re-using this.

if other is NaT:
# In this case we specifically interpret NaT as a datetime, not
# the timedelta interpretation we would get by returning self + NaT
result = self.asi8.view('m8[ms]') + NaT.to_datetime64()
return DatetimeArrayMixin(result)

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Oct 17, 2018

Author Member

This fixes the bug in TimedeltaIndex + np.timedelta64('NaT')

result = checked_add_with_arr(i8, other.value,
arr_mask=self._isnan)
result = self._maybe_mask_results(result, fill_value=iNaT)
return DatetimeArrayMixin(result, tz=other.tz)

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Oct 17, 2018

Author Member

This fixes a bug with failing to attach tz

@pytest.mark.parametrize('other', [pd.Timestamp.now(),
pd.Timestamp.now().to_pydatetime(),
pd.Timestamp.now().to_datetime64()])
def test_pi_add_sub_datetime(self, other):

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Oct 17, 2018

Author Member

This behavior is unchanged; it was just an untested case.

@TomAugspurger TomAugspurger added this to Mostly Orthogonal Pieces in DatetimeArray Refactor Oct 18, 2018

@codecov

This comment has been minimized.

Copy link

commented Oct 18, 2018

Codecov Report

Merging #23215 into master will decrease coverage by <.01%.
The diff coverage is 95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #23215      +/-   ##
==========================================
- Coverage   92.16%   92.16%   -0.01%     
==========================================
  Files         166      166              
  Lines       51224    51206      -18     
==========================================
- Hits        47212    47192      -20     
- Misses       4012     4014       +2
Flag Coverage Δ
#multiple 90.59% <95%> (-0.01%) ⬇️
#single 42.24% <21.66%> (+0.01%) ⬆️
Impacted Files Coverage Δ
pandas/core/arrays/datetimelike.py 94.01% <100%> (+0.79%) ⬆️
pandas/core/arrays/timedeltas.py 94.3% <100%> (-0.18%) ⬇️
pandas/core/indexes/timedeltas.py 90.62% <100%> (ø) ⬆️
pandas/core/indexes/period.py 91.04% <84.21%> (-1.19%) ⬇️
pandas/core/arrays/period.py 97.48% <93.54%> (-0.09%) ⬇️
pandas/core/arrays/datetimes.py 97.9% <96%> (+0.39%) ⬆️

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 62a15fa...0466b9c. Read the comment docs.

@pep8speaks

This comment has been minimized.

Copy link

commented Oct 18, 2018

Hello @jbrockmendel! Thanks for updating the PR.

file_to_check.py:2522:-258: W605 invalid escape sequence '('
file_to_check.py:2522:-255: W605 invalid escape sequence ')'


@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

@datapythonista where should I be looking for the explanation of why the linting is failing? It used to tell me right above the "exited wth code 1" line

From Travis log:

============== 4 passed, 4 deselected, 1 warnings in 0.24 seconds ==============
Doctests top-level reshaping functions DONE
The command "ci/code_checks.sh" exited with 1.
@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

https://travis-ci.org/pandas-dev/pandas/jobs/443307753#L2918

I search for ci/code_checks.sh. We should be passing a -q to the doctests I think.

@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

It's the imports I think?

Check import format using isort
ERROR: /home/travis/build/pandas-dev/pandas/pandas/core/indexes/range.py Imports are incorrectly sorted.
Check import format using isort DONE
@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

It's the imports I think?

Yah, I think #23231 should have just fixed it. I'll re-push in a bit.

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

Updated with additional bugfixes:

  • DateOffset + (PeriodIndex|DataFrame) no longer raises TypeError
  • PeriodIndex[freq.n!=1] + DateOffset is no longer incorrect

jbrockmendel added some commits Oct 26, 2018

@jreback

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

ping on green

jbrockmendel added some commits Oct 26, 2018

@jorisvandenbossche
Copy link
Member

left a comment

You did a rename of things like _sub_datelike_dti to _sub_datetime_arraylike, which is good, but the same could be done for _sub_delta_tdi and others (can maybe also be left for another PR, as those are touched less here)

Added some minor comments/questions, looks good to me for the rest

Show resolved Hide resolved pandas/core/arrays/period.py
Show resolved Hide resolved pandas/core/arrays/period.py
@@ -844,7 +753,7 @@ def _add_delta_td(self, other):
# delta_to_nanoseconds(delta). Because delta here is an integer,
# delta_to_nanoseconds will return it unchanged.
ordinals = super(PeriodArray, self)._add_delta_td(delta)
return type(self)(ordinals, self.freq)
return ordinals

def _add_delta_tdi(self, other):

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Oct 26, 2018

Member

The superclass version seems to return integers, but this an actual PeriodArray?

Can you add a comment indicating the types?

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Oct 26, 2018

Author Member

The superclass version seems to return integers, but this an actual PeriodArray?

Both this and the superclass version return i8 array. The superclass has a brief docstring to that effect.

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Oct 27, 2018

Member

Can you then add a comment on this function indicating the types? That really makes it easier to read through this code

But, to my original question: this function uses _addsub_int_array, which is defined a little bit above to return a PeriodArray, and not a i8 array.
I assume in practice this is not really a problem, because the code that converts the i8 array to PeriodArray, will also accept a PeriodArray.

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Oct 27, 2018

Author Member

Good catch, I must have lost track of this somewhere in the rebase process. Fixed to correctly return an i8 array, and brief docstrings added indicating types.

Show resolved Hide resolved pandas/core/arrays/period.py
@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2018

@jorisvandenbossche anything else to address or can i ping jeff?

@jreback

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

will look tonight

@jreback
Copy link
Contributor

left a comment

minor comments, needds a rebase, ping on green.

@@ -221,7 +221,7 @@ def hasnans(self):
""" return if I have any nans; enables various perf speedups """
return self._isnan.any()

def _maybe_mask_results(self, result, fill_value=None, convert=None):
def _maybe_mask_results(self, result, fill_value=iNaT, convert=None):
"""
Parameters
----------

This comment has been minimized.

Copy link
@jreback

jreback Oct 28, 2018

Contributor

can you update the doc-string for fill_value

# delta_to_nanoseconds(delta). Because delta here is an integer,
# delta_to_nanoseconds will return it unchanged.
result = super(PeriodArray, self)._add_delta_td(other.n)
return type(self)(result, freq=self.freq)

def _add_delta_td(self, other):

This comment has been minimized.

Copy link
@jreback

jreback Oct 28, 2018

Contributor

can you rename this to _add_timedelta? (or _add_delta_timedelta)

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Oct 28, 2018

Ping

@jreback jreback merged commit f0746fc into pandas-dev:master Oct 28, 2018

3 checks passed

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

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2018

thanks @jbrockmendel

@jbrockmendel jbrockmendel deleted the jbrockmendel:arith branch Oct 28, 2018

thoo added a commit to thoo/pandas that referenced this pull request Oct 30, 2018

Merge remote-tracking branch 'repo_org/master' into Add_ExtensionArra…
…y_tests

* repo_org/master: (52 commits)
  ENH: Allow rename_axis to specify index and columns arguments  (pandas-dev#20046)
  STY: proposed isort settings [ci skip] [skip ci] [ciskip] [skipci] (pandas-dev#23366)
  MAINT: Remove extraneous test.parquet file
  CLN: Follow-up comments to pandas-devgh-23392 (pandas-dev#23401)
  BUG GH23282 calling min on series of NaT returns NaT (pandas-dev#23289)
  unpin openpyxl (pandas-dev#23361)
  REF: collect ops dispatch functions in one place, try to de-duplicate SparseDataFrame methods (pandas-dev#23060)
  CLN: Remove pandas.tools module (pandas-dev#23376)
  CLN: Remove some dtype methods from API (pandas-dev#23390)
  CLN: Cleanup toplevel namespace shims (pandas-dev#23386)
  DOC: fixup whatsnew note for GH21394 (pandas-dev#23355)
  Fix import format at pandas/tests/extension directory (pandas-dev#23365)
  DOC: Remove Series.sortlevel from api.rst (pandas-dev#23395)
  API: Disallow dtypes w/o frequency when casting (pandas-dev#23392)
  BUG/TST/REF: Datetimelike Arithmetic Methods (pandas-dev#23215)
  STYLE: lint
  add np.nan* funcs to cython_table (pandas-dev#22109)
  Run Isort on tests/util single PR (pandas-dev#23347)
  BUG: Fix date_range overflow (pandas-dev#23345)
  Run Isort on tests/arrays single PR (pandas-dev#23346)
  ...

@TomAugspurger TomAugspurger moved this from Mostly Orthogonal Pieces to Done in DatetimeArray Refactor Nov 5, 2018

brute4s99 added a commit to brute4s99/pandas that referenced this pull request Nov 19, 2018

@jbrockmendel jbrockmendel removed this from Done in DatetimeArray Refactor Jan 4, 2019

Pingviinituutti added a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

Pingviinituutti added 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
You can’t perform that action at this time.