add freq as CFTimeIndex property and to CFTimeIndex.repr #4597

aaronspring · 2020-11-21T20:12:36Z

Closes Indicate calendar type in CFTimeIndex repr #2416
Tests added
Passes isort . && black . && mypy . && flake8
User visible changes (including notable bug fixes) are documented in whats-new.rst
New functions/methods are listed in api.rst

pep8speaks · 2020-11-21T20:12:41Z

Hello @aaronspring! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-11-23 16:02:32 UTC

aaronspring · 2020-11-21T21:10:44Z

I also get this error locally. somehow in some envs xarray (but not in others) tries to use daysinmonth. I also know about days_in_month, but just changing that in frequencies.py

xarray/xarray/coding/frequencies.py

Line 261 in d9ebcaf

cal = date.day == date.daysinmonth

didnt help it.

____________________________________________________________________________ test_cftimeindex_freq_in_repr[MS-noleap] _____________________________________________________________________________

freq = 'MS', calendar = 'noleap'

    @requires_cftime
    @pytest.mark.parametrize("calendar", ["noleap", "360_day"])
    @pytest.mark.parametrize("freq", ["1D", "MS"])
    def test_cftimeindex_freq_in_repr(freq,calendar):
        index = xr.cftime_range(start="2000", periods=3, freq=freq, calendar=calendar)
>       assert f', freq={freq}' in index.__repr__()

/Users/aaron.spring/Coding/xarray/xarray/tests/test_cftimeindex.py:952:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/Users/aaron.spring/Coding/xarray/xarray/coding/cftimeindex.py:342: in __repr__
    attrs_str = format_attrs(self)
/Users/aaron.spring/Coding/xarray/xarray/coding/cftimeindex.py:263: in format_attrs
    "freq": f"'{index.freq}'"
/Users/aaron.spring/Coding/xarray/xarray/coding/cftimeindex.py:691: in freq
    return infer_freq(self)
/Users/aaron.spring/Coding/xarray/xarray/coding/frequencies.py:97: in infer_freq
    return inferer.get_freq()
/Users/aaron.spring/Coding/xarray/xarray/coding/frequencies.py:132: in get_freq
    return self._infer_daily_rule()
/Users/aaron.spring/Coding/xarray/xarray/coding/frequencies.py:165: in _infer_daily_rule
    monthly_rule = self._get_monthly_rule()
/Users/aaron.spring/Coding/xarray/xarray/coding/frequencies.py:199: in _get_monthly_rule
    return {"cs": "MS", "ce": "M"}.get(month_anchor_check(self.index))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

dates = <[AttributeError("'cftime._cftime.DatetimeNoLeap' object has no attribute 'daysinmonth'",) raised in repr()] CFTimeIndex object at 0x7fca9312b160>

    def month_anchor_check(dates):
        """Return the monthly offset string.

        Return "cs" if all dates are the first days of the month,
        "ce" if all dates are the last day of the month,
        None otherwise.

        Replicated pandas._libs.tslibs.resolution.month_position_check
        but without business offset handling.
        """
        calendar_end = True
        calendar_start = True

        for date in dates:
            if calendar_start:
                calendar_start &= date.day == 1

            if calendar_end:
>               cal = date.day == date.daysinmonth
E               AttributeError: 'cftime._cftime.DatetimeNoLeap' object has no attribute 'daysinmonth'

/Users/aaron.spring/Coding/xarray/xarray/coding/frequencies.py:261: AttributeError
____________________________________________________________________________ test_cftimeindex_freq_in_repr[MS-360_day] ____________________________________________________________________________

freq = 'MS', calendar = '360_day'

    @requires_cftime
    @pytest.mark.parametrize("calendar", ["noleap", "360_day"])
    @pytest.mark.parametrize("freq", ["1D", "MS"])
    def test_cftimeindex_freq_in_repr(freq,calendar):
        index = xr.cftime_range(start="2000", periods=3, freq=freq, calendar=calendar)
>       assert f', freq={freq}' in index.__repr__()

/Users/aaron.spring/Coding/xarray/xarray/tests/test_cftimeindex.py:952:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/Users/aaron.spring/Coding/xarray/xarray/coding/cftimeindex.py:342: in __repr__
    attrs_str = format_attrs(self)
/Users/aaron.spring/Coding/xarray/xarray/coding/cftimeindex.py:263: in format_attrs
    "freq": f"'{index.freq}'"
/Users/aaron.spring/Coding/xarray/xarray/coding/cftimeindex.py:691: in freq
    return infer_freq(self)
/Users/aaron.spring/Coding/xarray/xarray/coding/frequencies.py:97: in infer_freq
    return inferer.get_freq()
/Users/aaron.spring/Coding/xarray/xarray/coding/frequencies.py:132: in get_freq
    return self._infer_daily_rule()
/Users/aaron.spring/Coding/xarray/xarray/coding/frequencies.py:165: in _infer_daily_rule
    monthly_rule = self._get_monthly_rule()
/Users/aaron.spring/Coding/xarray/xarray/coding/frequencies.py:199: in _get_monthly_rule
    return {"cs": "MS", "ce": "M"}.get(month_anchor_check(self.index))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

dates = <[AttributeError("'cftime._cftime.Datetime360Day' object has no attribute 'daysinmonth'",) raised in repr()] CFTimeIndex object at 0x7fca930cfe10>

    def month_anchor_check(dates):
        """Return the monthly offset string.

        Return "cs" if all dates are the first days of the month,
        "ce" if all dates are the last day of the month,
        None otherwise.

        Replicated pandas._libs.tslibs.resolution.month_position_check
        but without business offset handling.
        """
        calendar_end = True
        calendar_start = True

        for date in dates:
            if calendar_start:
                calendar_start &= date.day == 1

            if calendar_end:
>               cal = date.day == date.daysinmonth
E               AttributeError: 'cftime._cftime.Datetime360Day' object has no attribute 'daysinmonth'

/Users/aaron.spring/Coding/xarray/xarray/coding/frequencies.py:261: AttributeError
===================================================================================== short test summary info =====================================================================================
FAILED xarray/tests/test_cftimeindex.py::test_cftimeindex_freq_in_repr[1D-noleap] - AttributeError: 'cftime._cftime.DatetimeNoLeap' object has no attribute 'daysinmonth'
FAILED xarray/tests/test_cftimeindex.py::test_cftimeindex_freq_in_repr[1D-360_day] - AttributeError: 'cftime._cftime.Datetime360Day' object has no attribute 'daysinmonth'
FAILED xarray/tests/test_cftimeindex.py::test_cftimeindex_freq_in_repr[MS-noleap] - AttributeError: 'cftime._cftime.DatetimeNoLeap' object has no attribute 'daysinmonth'
FAILED xarray/tests/test_cftimeindex.py::test_cftimeindex_freq_in_repr[MS-360_day] - AttributeError: 'cftime._cftime.Datetime360Day' object has no attribute 'daysinmonth'
======================================================================================== 4 failed in 2.58s ===========================================================================

keewis · 2020-11-21T21:25:30Z

somehow in some envs xarray (but not in others)

Actually, there's no inconsistency here: py38-flaky fails, but it is allowed to fail so we still get a green check mark (#4584 slightly changes that). py36-min-nep18 does not install cftime so it doesn't fail, either. Doctests fails because it creates and prints CFTimeIndex objects with 1 or 2 values (and I think there's also a test that does something similar).

To fix the CI, you might need to fix both the daysinmonth and allow CFTimeIndex objects with less than 3 values.

aaronspring · 2020-11-21T22:04:11Z

also should it be frequency or freq?

keewis · 2020-11-21T22:09:15Z

the parameter is named freq so I think it should be fine to use that, but I don't have a strong opinion on this

spencerkclark · 2020-11-21T22:19:12Z

freq would be consistent with pandas:

In [1]: import pandas as pd

In [2]: pd.date_range("2000", periods=3)
Out[2]: DatetimeIndex(['2000-01-01', '2000-01-02', '2000-01-03'], dtype='datetime64[ns]', freq='D')

@aaronspring regarding the local test failure -- which version of cftime are you using in your environment? The daysinmonth attribute did not always exist; it was added in version 1.1.0.

keewis · 2020-11-21T22:22:27Z

@spencerclark, see the build logs of the py36-min-all-deps CI, we can't use version 1.1.0 unless we bump the version of cftime

spencerkclark · 2020-11-21T22:27:34Z

Thanks @keewis I didn't realize this was also happening in CI. Yeah, the version is definitely the problem:

cftime                    1.0.4.2          py36hc1659b7_0    conda-forge

My vote for now would be to skip these tests on versions less than 1.1.0. Perhaps we could also think about adding a nicer error message if the version is not up-to-date enough.

~~I guess our test coverage of infer_freq wasn't good enough to trip this up already.~~ Oops we already have a precedent of skipping the infer_freq tests that require this, e.g.:

xarray/xarray/tests/test_cftimeindex.py

Lines 1180 to 1192 in a219215

    
           @requires_cftime_1_1_0 
        
           def test_infer_freq_valid_types(): 
        
               cf_indx = xr.cftime_range("2000-01-01", periods=3, freq="D") 
        
               assert xr.infer_freq(cf_indx) == "D" 
        
               assert xr.infer_freq(xr.DataArray(cf_indx)) == "D" 
        
               pd_indx = pd.date_range("2000-01-01", periods=3, freq="D") 
        
               assert xr.infer_freq(pd_indx) == "D" 
        
               assert xr.infer_freq(xr.DataArray(pd_indx)) == "D" 
        
               pd_td_indx = pd.timedelta_range(start="1D", periods=3, freq="D") 
        
               assert xr.infer_freq(pd_td_indx) == "D" 
        
               assert xr.infer_freq(xr.DataArray(pd_td_indx)) == "D"

aaronspring · 2020-11-22T10:11:53Z

Ah I need to account for the docstring examples with only one or two items, maybe dropping freq then

xarray/tests/test_cftimeindex.py

xarray/coding/cftimeindex.py

spencerkclark

Thanks @aaronspring; this looks great. Just a few small things. Could you also add CFTimeIndex.freq to doc/api-hidden.rst?

xarray/coding/cftimeindex.py

xarray/tests/test_cftimeindex.py

doc/api-hidden.rst

spencerkclark

Nice, this looks good to me. I agree with @keewis that it would be good to move CFTimeIndex.freq to the group of CFTimeIndex attributes in api-hidden.rst, but it is not strictly necessary.

keewis

thanks, @aaronspring, looks good to me, too. Let's merge?

dcherian · 2020-11-24T21:53:12Z

LGTM too. Thanks @aaronspring

aaronspring · 2020-11-25T09:16:49Z

Thanks for the smooth review process @spencerkclark @keewis

add freq as property and to repr

0ca1cca

aaronspring changed the title ~~add freq as property and to repr~~ add freq as CFTimeIndex property and to CFTimeIndex.__repr__ Nov 21, 2020

lint

c0af512

Update test_cftimeindex.py

591d23d

Update test_cftimeindex.py

a43e4e9

require cftime110

c661ba1

aaronspring added 5 commits November 22, 2020 11:15

Only show freq if more than two items

71e017d

Update test_cftimeindex.py

d89bd9f

Update cftime_offsets.py

f3b750d

Require cftime110 for all repr tests

ddd22e1

Update test_cftimeindex.py

662fcdf

aaronspring commented Nov 22, 2020

View reviewed changes

xarray/tests/test_cftimeindex.py Outdated Show resolved Hide resolved

aaronspring added 2 commits November 22, 2020 12:50

Update xarray/tests/test_cftimeindex.py

83af932

Update test_cftimeindex.py

31cb3db

aaronspring commented Nov 22, 2020

View reviewed changes

xarray/tests/test_cftimeindex.py Outdated Show resolved Hide resolved

aaronspring and others added 2 commits November 22, 2020 15:30

Update xarray/tests/test_cftimeindex.py

c4ace5b

final lint

d610a2f

aaronspring marked this pull request as ready for review November 22, 2020 21:07

aaronspring commented Nov 22, 2020

View reviewed changes

xarray/coding/cftimeindex.py Outdated Show resolved Hide resolved

spencerkclark reviewed Nov 22, 2020

View reviewed changes

freq in repr always and add to hidden-api

5a75923

AS added 2 commits November 23, 2020 09:04

fix doctests by adding freq=None

c8c41b2

rerun CI after timeout

c3c8531

aaronspring commented Nov 23, 2020

View reviewed changes

doc/api-hidden.rst Outdated Show resolved Hide resolved

aaronspring requested a review from spencerkclark November 23, 2020 11:41

spencerkclark approved these changes Nov 23, 2020

View reviewed changes

AS added 2 commits November 23, 2020 16:46

api-hidden

c317398

rerun CI

52ee89a

keewis approved these changes Nov 24, 2020

View reviewed changes

dcherian merged commit 16b15db into pydata:master Nov 24, 2020

Uh oh!

add freq as CFTimeIndex property and to CFTimeIndex.__repr__ #4597

add freq as CFTimeIndex property and to CFTimeIndex.__repr__ #4597

Uh oh!

Conversation

aaronspring commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-11-23 16:02:32 UTC

Uh oh!

aaronspring commented Nov 21, 2020

Uh oh!

keewis commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronspring commented Nov 21, 2020

Uh oh!

keewis commented Nov 21, 2020

Uh oh!

spencerkclark commented Nov 21, 2020

Uh oh!

keewis commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spencerkclark commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronspring commented Nov 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spencerkclark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spencerkclark left a comment

Choose a reason for hiding this comment

Uh oh!

keewis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcherian commented Nov 24, 2020

Uh oh!

aaronspring commented Nov 25, 2020

Uh oh!

Uh oh!

add freq as CFTimeIndex property and to CFTimeIndex.repr #4597

add freq as CFTimeIndex property and to CFTimeIndex.repr #4597

aaronspring commented Nov 21, 2020 •

edited

Loading

pep8speaks commented Nov 21, 2020 •

edited

Loading

keewis commented Nov 21, 2020 •

edited

Loading

keewis commented Nov 21, 2020 •

edited

Loading

spencerkclark commented Nov 21, 2020 •

edited

Loading

keewis left a comment •

edited

Loading