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

Align long time interval check with cftime #274

Closed
trexfeathers opened this issue Jul 12, 2022 · 1 comment · Fixed by #279
Closed

Align long time interval check with cftime #274

trexfeathers opened this issue Jul 12, 2022 · 1 comment · Fixed by #279

Comments

@trexfeathers
Copy link
Collaborator

We currently blanket disallow month or year intervals when passing operations down to cftime:

cf-units/cf_units/__init__.py

Lines 1932 to 1939 in 37c8400

# `cftime` cannot parse long time intervals ("months" or "years").
if self.is_long_time_interval():
interval = self.origin.split(" ")[0]
emsg = (
'Time units with interval of "months", "years" '
'(or singular of these) cannot be processed, got "{!s}".'
)
raise ValueError(emsg.format(interval))

cftime disallows these intervals, presumably because it requires consistent-length time increments. But cftime is more intelligent about this than cf-units is, e.g. month intervals are allowed for 360_day calendars (Unidata/cftime#75).

We should make sure our own checks don't cover over details like the 360_day exception. The Pythonic choice would be to do no extra checks at all in cf-units, simply relying on cftime to raise the appropriate error; is there a good reason why we don't do this?

@rcomer
Copy link
Member

rcomer commented Jul 12, 2022

Looks like the decision was made in cf-units ages ago #72. Seems sensible to revisit.

@trexfeathers trexfeathers self-assigned this Jul 20, 2022
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Oct 18, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Oct 24, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Oct 25, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Oct 26, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Oct 27, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Oct 28, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Oct 29, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Oct 30, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Oct 31, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Nov 1, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Nov 2, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Nov 3, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Nov 4, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Nov 5, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Nov 6, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Nov 7, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants