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

Add support for 'common_year' and 'common_years' units #246

Merged
merged 3 commits into from
Jun 1, 2021

Conversation

andersy005
Copy link
Contributor

This PR is my attempt at fixing #5. As discussed in #5, the only supported calendars for year units are 365_day and noleap.

In [1]: import cftime

In [2]: values = [1, 2, 3]

In [3]: units = 'common_year since 0000-01-01 0:0:0'

In [4]: cftime.num2date(values, units, 'noleap')
Out[4]: 
array([cftime.DatetimeNoLeap(1, 1, 1, 0, 0, 0, 0, has_year_zero=True),
       cftime.DatetimeNoLeap(2, 1, 1, 0, 0, 0, 0, has_year_zero=True),
       cftime.DatetimeNoLeap(3, 1, 1, 0, 0, 0, 0, has_year_zero=True)],
      dtype=object)

In [5]: cftime.num2date(values, units, '365_day')
Out[5]: 
array([cftime.DatetimeNoLeap(1, 1, 1, 0, 0, 0, 0, has_year_zero=True),
       cftime.DatetimeNoLeap(2, 1, 1, 0, 0, 0, 0, has_year_zero=True),
       cftime.DatetimeNoLeap(3, 1, 1, 0, 0, 0, 0, has_year_zero=True)],
      dtype=object)

Using common_year or years units with other calendars results in failures as intended:

n [6]: cftime.num2date(values, units, 'standard')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-6-5819edfbac07> in <module>
----> 1 cftime.num2date(values, units, 'standard')

~/devel/unidata/cftime/src/cftime/_cftime.pyx in cftime._cftime.num2date()
    488     if has_year_zero is None:
    489         has_year_zero = _year_zero_defaults(calendar)
--> 490     basedate = _dateparse(units,calendar=calendar,has_year_zero=has_year_zero)
    491 
    492     can_use_python_datetime=_can_use_python_datetime(basedate,calendar)

~/devel/unidata/cftime/src/cftime/_cftime.pyx in cftime._cftime._dateparse()
     99             raise ValueError("'months since' units only allowed for '360_day' calendar")
    100         if units in year_units and calendar not in {'365_day', 'noleap'}:
--> 101             raise ValueError("'%s' units only allowed for '365_day' and 'noleap' calendars" % units)
    102         else:
    103             raise ValueError(

ValueError: 'common_year' units only allowed for '365_day' and 'noleap' calendars

@kmpaul, let me know if this PR fully addresses #5.

@CLAassistant
Copy link

CLAassistant commented May 28, 2021

CLA assistant check
All committers have signed the CLA.

@jswhit
Copy link
Collaborator

jswhit commented May 29, 2021

According to the udunits docs a common_year is 365 days, but a year is 365.242198781 days. In addition, a leap_year is 366 days, a julian_year is 365.25 days and a gregorian_year is 365.2425 days.

So, I think there may some ambiguity in allowing years since - does it mean 365 or 365.242198781 days? I guess that's why they advise against using years as a unit.

@jswhit
Copy link
Collaborator

jswhit commented May 29, 2021

seems like if it works for 'noleap', 'years since' should also be supported for 'all_leap' and '360_day' calendars

@jswhit
Copy link
Collaborator

jswhit commented May 30, 2021

Allowing 'year' units is a rabbit hole I don't think we really want to go all the way down. The only examples of netcdf files with that use the 'common_years' time units in the wild that I can find seem to be output from the ice-sheet model in CESM, which uses the '365_day' calendar. I'm willing to merge this PR to deal with this particular use-case, but after thinking about this some more I don't think enabling this for other calendars is a good idea.

@jswhit
Copy link
Collaborator

jswhit commented May 30, 2021

Needs a Changelog entry

@andersy005
Copy link
Contributor Author

So, I think there may some ambiguity in allowing years since - does it mean 365 or 365.242198781 days? I guess that's why they advise against using years as a unit.
Allowing 'year' units is a rabbit hole I don't think we really want to go all the way down.

Thank you for your feedback, @jswhit! I concur. I'm going to remove the years units and only keep common_year units and will update the changelog.

src/cftime/_cftime.pyx Outdated Show resolved Hide resolved
src/cftime/_cftime.pyx Outdated Show resolved Hide resolved
@andersy005 andersy005 changed the title Add support for 'common_year' and 'years' units Add support for 'common_year' and 'common_years' units Jun 1, 2021
@andersy005
Copy link
Contributor Author

@jswhit, the CI isn't running because this is my first time contribution to cftime. could you approve running workflows when you get a moment?

test/test_cftime.py Outdated Show resolved Hide resolved
@jswhit jswhit merged commit 414c946 into Unidata:master Jun 1, 2021
@andersy005 andersy005 deleted the support-year-units branch June 1, 2021 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants