[TEST] Add two more parameters to the test_dti_add_sub_nonzero_mth_offset #26392

makbigc · 2019-05-14T15:32:51Z

follow up the comment in [BUG] Adding offset with nonzero month to DatetimeIndex (GH26258) #26292
passes git diff upstream/master -u -- "*.py" | flake8 --diff

codecov · 2019-05-14T16:18:31Z

Codecov Report

Merging #26392 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #26392      +/-   ##
==========================================
- Coverage   91.69%   91.68%   -0.01%     
==========================================
  Files         174      174              
  Lines       50749    50749              
==========================================
- Hits        46534    46530       -4     
- Misses       4215     4219       +4

Flag	Coverage Δ
#multiple	`90.19% <ø> (ø)`	⬆️
#single	`41.16% <ø> (-0.15%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️
pandas/core/frame.py	`97.02% <0%> (-0.12%)`	⬇️

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 612c244...5c35afe. Read the comment docs.

codecov · 2019-05-14T16:18:42Z

Codecov Report

Merging #26392 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #26392      +/-   ##
==========================================
- Coverage   91.75%   91.74%   -0.01%     
==========================================
  Files         174      174              
  Lines       50761    50761              
==========================================
- Hits        46574    46571       -3     
- Misses       4187     4190       +3

Flag	Coverage Δ
#multiple	`90.25% <ø> (ø)`	⬆️
#single	`41.61% <ø> (-0.17%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️
pandas/core/frame.py	`97.02% <0%> (-0.12%)`	⬇️
pandas/util/testing.py	`90.7% <0%> (+0.1%)`	⬆️

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 7b92e7e...7decf16. Read the comment docs.

jreback · 2019-05-14T17:15:44Z

lgtm. @jbrockmendel

jbrockmendel · 2019-05-14T17:25:55Z

pandas/tests/arithmetic/test_datetime64.py

        mth = getattr(date, op)
        result = mth(offset)
-        tm.assert_equal(result, exp)
+
+        expected = pd.Index(exp, tz=tz)


DateTimeIndex

any reason not to do this? passing tz to Index is allowed, but not encouraged

jbrockmendel · 2019-05-14T17:26:32Z

pandas/tests/arithmetic/test_datetime64.py

@@ -1435,27 +1435,38 @@ def test_dt64arr_add_sub_offset_ndarray(self, tz_naive_fixture,
        expected = tm.box_expected(expected, box_with_array)
        tm.assert_equal(res, expected)

+    @pytest.mark.parametrize("box", [pd.Index, pd.Series, pd.DataFrame])


Can you use box_with_array fixture?

In the case to_array of box_with_array, the DatetimeArray after addition of an offset of month and day doesn't have any freq. But adding an offset of month only results in freq. May I assert the equality of array values only by assert_numpy_array_equal?

In [18]: arr = pd.date_range(start='01 Jan 2014', end='01 Jan 2017', freq='AS'). ...: array In [19]: arr Out[19]: <DatetimeArray> ['2014-01-01 00:00:00', '2015-01-01 00:00:00', '2016-01-01 00:00:00', '2017-01-01 00:00:00'] Length: 4, dtype: datetime64[ns] In [20]: offset1 = pd.DateOffset(months=3, days=10) In [21]: offset1 Out[21]: <DateOffset: days=10, months=3> In [22]: (arr + offset1).freq is None Out[22]: True In [23]: offset2 = pd.DateOffset(months=3) In [24]: (arr + offset2).freq Out[24]: <YearBegin: month=4>

In the case to_array of box_with_array, the DatetimeArray after addition of an offset of month and day doesn't have any freq.

If DatetimeIndex + DateOffset behaves differently from DatetimeArray + DateOffset, that's a bug and should be fixed.

The difference starts at L384:

pandas/pandas/core/arrays/datetimes.py

Lines 381 to 384 in d3a1912

elif freq_infer:

# Set _freq directly to bypass duplicative _validate_frequency

# check.

result._freq = to_offset(result.inferred_freq)

There is a frequency AS-JAN defined to denote the beginning of Jan, but no frequency defined for a specific date.
ref.: https://pandas.pydata.org/pandas-docs/stable/user_guide/timeseries.html#anchored-offsets

In [2]: arr1 = pd.DatetimeIndex(['01 Jan 2014', '01 Jan 2015', '01 Jan 2016', '0 ...: 1 Jan 2017']).array In [3]: arr2 = pd.DatetimeIndex(['11 Apr 2014', '11 Apr 2015', '11 Apr 2016', '1 ...: 1 Apr 2017']).array In [4]: arr1.inferred_freq Out[4]: 'AS-JAN' In [5]: arr2.inferred_freq In [6]: arr2.inferred_freq is None Out[6]: True

We can set result._freq to DateOffset(month, day), when inferred_freq returns None. Is it what we want? (This change would break 19 tests in test_datetime64.py)

I think there was a miscommunication and I got confused. Hopefully I've got it right now, let's double-check. Thanks for bearing with me here.

The following should be true for any DatetimeIndex dti and any DateOffset offset:

(dti + offset).freq == (dti.array + offset).freq

Are you saying there is a counter-example? If not, I don't see why you can't use box_with_array.

(dti + offset).freq == (dti.array + offset).freq
This equality holds.

I add the freq check of the Index myself, because assert_index_equal doesn't check the freq. The freq of the Index match the expected frequency as the array does.

Got it, thanks.

I think there is an Issue for having assert_index_equal check freq for appropriate subclasses, if that's something you're interested in fixing.

pandas/tests/arithmetic/test_datetime64.py

jreback · 2019-05-18T14:40:29Z

@jbrockmendel merge if ok with the responses.

makbigc · 2019-05-24T03:18:29Z

@jbrockmendel Please tell me if you have anything to implement?

jbrockmendel · 2019-05-24T03:27:55Z

@makbigc can you clarify here whether there is a counter-example?

jbrockmendel · 2019-05-25T19:24:34Z

LGTM, merging.

jbrockmendel · 2019-05-25T19:24:47Z

Thanks @makbigc

…fset (pandas-dev#26392) * Add two more parameters to the test * Add array into the boy and add parameter freq

jreback added Frequency DateOffsets Testing pandas testing functions or related to the test suite Datetime Datetime data dtype labels May 14, 2019

jreback added this to the 0.25.0 milestone May 14, 2019

jbrockmendel reviewed May 14, 2019

View reviewed changes

pandas/tests/arithmetic/test_datetime64.py Outdated Show resolved Hide resolved

makbigc added 2 commits May 22, 2019 14:26

Add two more parameters to the test

91c25c8

Add array into the boy and add parameter freq

7decf16

makbigc force-pushed the test-26292 branch from 5c35afe to 7decf16 Compare May 22, 2019 06:28

jbrockmendel merged commit f8ace5f into pandas-dev:master May 25, 2019

makbigc deleted the test-26292 branch May 26, 2019 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEST] Add two more parameters to the test_dti_add_sub_nonzero_mth_offset #26392

[TEST] Add two more parameters to the test_dti_add_sub_nonzero_mth_offset #26392

makbigc commented May 14, 2019

codecov bot commented May 14, 2019

codecov bot commented May 14, 2019 •

edited

Loading

jreback commented May 14, 2019

jbrockmendel May 14, 2019

jbrockmendel May 18, 2019

jbrockmendel May 14, 2019

makbigc May 15, 2019

jbrockmendel May 18, 2019

makbigc May 21, 2019 •

edited

Loading

jbrockmendel May 22, 2019

makbigc May 24, 2019

jbrockmendel May 25, 2019

jreback commented May 18, 2019

makbigc commented May 24, 2019

jbrockmendel commented May 24, 2019

jbrockmendel commented May 25, 2019

jbrockmendel commented May 25, 2019

	elif freq_infer:
	# Set _freq directly to bypass duplicative _validate_frequency
	# check.
	result._freq = to_offset(result.inferred_freq)

[TEST] Add two more parameters to the test_dti_add_sub_nonzero_mth_offset #26392

[TEST] Add two more parameters to the test_dti_add_sub_nonzero_mth_offset #26392

Conversation

makbigc commented May 14, 2019

codecov bot commented May 14, 2019

Codecov Report

codecov bot commented May 14, 2019 • edited Loading

Codecov Report

jreback commented May 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makbigc May 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented May 18, 2019

makbigc commented May 24, 2019

jbrockmendel commented May 24, 2019

jbrockmendel commented May 25, 2019

jbrockmendel commented May 25, 2019

codecov bot commented May 14, 2019 •

edited

Loading

makbigc May 21, 2019 •

edited

Loading