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

Support gregorian to standard calendar change #205

Merged
merged 4 commits into from
Feb 16, 2022

Conversation

larsbarring
Copy link
Contributor

🚀 Pull Request

Description

Change definitive form of calendar from gregorian to standard, instead of the other way around.
With first time attempt at [change of ] unit tests for this.

@trexfeathers
Copy link
Collaborator

Thanks for submitting this @larsbarring! I was taking a look and I wondered if the changes to __init__ really would be that simple - I'm glad you think the same 🙂

@larsbarring
Copy link
Contributor Author

Yeah, but it remains to be seen. The unit tests fails miserably (I am a complete rookie at these). And I have no insight regarding possible downstream dependencies

@trexfeathers
Copy link
Collaborator

The unit tests fails miserably (I am a complete rookie at these). And I have no insight regarding possible downstream dependencies

The traceback is the same error we have been seeing recently during documentation builds, so we can definitely say it's not resulting in changes you have made. Unfortunately I am also clueless to the fix at the moment!

@trexfeathers
Copy link
Collaborator

I've asked if @bjlittle could take a look at this - the problem was first observed at 5540e08. But we're quite limited on time at the moment so there might be a wait.

@larsbarring
Copy link
Contributor Author

Thanks, I have something for #185 in the pipeline, but it uses this change.

@larsbarring
Copy link
Contributor Author

@trexfeathers @bjlittle Any chance the unit test problem might be fixed reasonably soon? That is, soon enough that the upcoming iris 3.2.0 will use cf-units with this fix implemented.

@trexfeathers
Copy link
Collaborator

@trexfeathers @bjlittle Any chance the unit test problem might be fixed reasonably soon? That is, soon enough that the upcoming iris 3.2.0 will use cf-units with this fix implemented.

We've seen your message 🙂. We'll have a better idea of the answer in the next week.

@wjbenfold
Copy link
Contributor

Rebasing onto main should resolve the unit test problem @larsbarring

@trexfeathers
Copy link
Collaborator

Rebasing onto main should resolve the unit test problem @larsbarring

I've just been checking locally and there are some actual unit test failures relating to the changes, but they look fairly simple 🙂 We'll see the detail once this is rebased.

@larsbarring
Copy link
Contributor Author

The linting failure I am not sure what to do about. And now two of the three initial test failures have been fixed. The third one is a bit more complicated. It seems that in Units.__num2date_to_nearest_second cftime returns an instance of cftime.DatetimeGregorian despite that the calendar specified in the call is "standard", cf. cftime issue #256 and PR #257. Maybe this could be an issue elsewhere where cftime is called?

@rcomer
Copy link
Member

rcomer commented Jan 13, 2022

The linting failure I am not sure what to do about

I think maybe we need to do for cf_units what SciTools/iris#4235 did for Iris. That's a bit outside my area of competency though!

@trexfeathers
Copy link
Collaborator

The linting failure I am not sure what to do about

@larsbarring are you using pre-commit/black locally? I know we haven't made this expectation obvious.

@larsbarring
Copy link
Contributor Author

hrm ... probably not as it seems, at least not correctly configured ....

@trexfeathers
Copy link
Collaborator

@larsbarring if you install pre-commit in your development environment I think it should "just work" whenever you make a commit 🤞

@larsbarring
Copy link
Contributor Author

Hm, this is becoming a bit too complex. The pre-commit black gives the same uninformative error, and seems to have reformatted lines that I have not touched L. 846-849, as well as L 1816-1819 where I changed the calendar.

@trexfeathers
Copy link
Collaborator

I see what @rcomer is saying now. I've asked if anyone in the team has some time to look at this.

Thanks, as ever, for your patience @larsbarring 😬

@trexfeathers
Copy link
Collaborator

The linting failure I am not sure what to do about

I think maybe we need to do for cf_units what SciTools/iris#4235 did for Iris. That's a bit outside my area of competency though!

Done, thanks to @wjbenfold ❤. @larsbarring if you bring in the latest main this ought to fix the linting trouble.

@larsbarring
Copy link
Contributor Author

larsbarring commented Jan 14, 2022

Linting failure solved -- thanks @wjbenfold, @trexfeathers, @rcomer !

The remaining test failure is because of Unit.num2date returns gregorian calendar:

in Units.__num2date_to_nearest_second cftime returns an instance of cftime.DatetimeGregorian despite that the calendar specified in the call is "standard",

Here is a quick hack to show the problem:

import cftime
import cf_units as unit
Unit = unit.Unit
u = Unit(
   "hours since 2010-11-02 12:00:00", calendar=unit.CALENDAR_GREGORIAN
)
print("repr(u)      =  ", repr(u))
res = u.num2date(1)
print("repr(res)    =  ", repr(res))
print("res.calendar =  ", res.calendar)

# repr(u)      =   Unit('hours since 2010-11-02 12:00:00', calendar='standard')
# repr(res)    =   cftime.DatetimeGregorian(2010, 11, 2, 13, 0, 0, 0, has_year_zero=False)
# res.calendar =   gregorian

And I do not know whether to wait for cftime to do something, or how fix this within iris in a safe and general way.

@rcomer
Copy link
Member

rcomer commented Jan 15, 2022

As far as I can tell, it's impossible to create a standard calendar datetime instance with the current cftime release:

In [4]: cftime.datetime(2022, 1, 1, calendar="standard")
Out[4]: cftime.datetime(2022, 1, 1, 0, 0, 0, 0, calendar='gregorian', has_year_zero=False)

In [5]: test = _

In [6]: test.calendar = "standard"
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-6-fc68e10f067c> in <module>
----> 1 test.calendar = "standard"

AttributeError: attribute 'calendar' of 'cftime._cftime.datetime' objects is not writable

So I think the only way to get num2date to return a standard calendar object is to wait for the next cftime release, which will include the changes from Unidata/cftime#257. At that point I think it will change to "standard" regardless of what we do here.

Perhaps it doesn't matter. Since you are looking at the CF Conventions, I guess your main concern is how NetCDF files are written? I have an existing NetCDF file that was saved by a previous Iris version. The time coordinate looks like this with ncdump:

	double time(forecast_reference_time) ;
		time:bounds = "time_bnds" ;
		time:units = "hours since 1970-01-01 00:00:00" ;
		time:standard_name = "time" ;
		time:calendar = "gregorian" ;

I made an environment with your branch and Iris v3.1. I used this to load and re-save my file. Now the ncdump shows me this:

	double time(forecast_reference_time) ;
		time:bounds = "time_bnds" ;
		time:units = "hours since 1970-01-01 00:00:00" ;
		time:standard_name = "time" ;
		time:calendar = "standard" ;

So for NetCDF saving, the change seems to be having the desired effect. So one option may be to revert the expected value of the num2date test and bank the rest of the change.

There may be other considerations that I am missing. What do others think?

@larsbarring
Copy link
Contributor Author

My main reason for pushing this issue, except for the general argument that it is nice to follow CF, is that we need to concatenate data from different sources that happens to have different calendars (#185). But in a PR to solve that issue I could include tests of both calendars (that later could compacted once a new version of cftime is available).

@rcomer
Copy link
Member

rcomer commented Jan 26, 2022

@SciTools/cf-units-devs I believe these tests should now pass with the latest version of cftime. I tried re-running the py39 tests this morning but that didn't help. Is there a way to make the tests start again with a fresh environment?

@bjlittle
Copy link
Member

TBH not sure we can get this banked in time for iris 3.2 (which is our main focus at the moment), but we can swarm on it afterwards, as we need to make sure we get this right.

If banking this PR forces subsequent changes upstream within iris, then we can opt to release an iris 3.2.1 patch to align everything.

Sound like a reasonable plan?

@wjbenfold
Copy link
Contributor

I believe these tests should now pass with the latest version of cftime. I tried re-running the py39 tests this morning but that didn't help. Is there a way to make the tests start again with a fresh environment?

It looks like if a py3.9 lockfile already exists, it refuses to overwrite it even if the build number gets bumped, so I'm not sure @rcomer

@wjbenfold
Copy link
Contributor

Ok, I think if you rebase onto main then we can hopefully get the CI passing @larsbarring. Then we can move to reviewing with confidence that what's here passes the tests

@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #205 (b9c01a3) into main (a210133) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #205   +/-   ##
=======================================
  Coverage   91.05%   91.05%           
=======================================
  Files           6        6           
  Lines         816      816           
  Branches      121      121           
=======================================
  Hits          743      743           
  Misses         61       61           
  Partials       12       12           
Impacted Files Coverage Δ
cf_units/__init__.py 89.72% <100.00%> (ø)

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 1d46964...b9c01a3. Read the comment docs.

@larsbarring
Copy link
Contributor Author

Pretty nice to see this passing the checks. Thanks all, for helping me with setting up these these tools :-)

Copy link
Member

@bjlittle bjlittle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@larsbarring Awesome! Thanks so much 💯 🍻

@bjlittle bjlittle merged commit f6ee3e3 into SciTools:main Feb 16, 2022
@larsbarring larsbarring deleted the fix_issue_203 branch February 16, 2022 14:24
@wjbenfold
Copy link
Contributor

If banking this PR forces subsequent changes upstream within iris, then we can opt to release an iris 3.2.1 patch to align everything.

Note for the interested - having run the Iris tests with current cf-units main (with this change) and current Iris, the tests all pass. We can jump up and down on it a bit more after the next cf-units release to double-check but it's looking good :)

@rcomer
Copy link
Member

rcomer commented Feb 22, 2022

I've also tried running the Iris tests with this and did get failures, so not sure what we're doing differently. The good news is those failures were easy to fix with find-and-replace within Iris.

@bjlittle bjlittle changed the title Fix issue 203 Support gregorian to standard calendar change Jul 1, 2022
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.

Change definitive calendar form from "gregorian" to "standard"
5 participants