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

Cell comparison: remove ancient netcdftime/cftime workarounds #4729

Merged
merged 2 commits into from
Oct 7, 2022

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented May 5, 2022

🚀 Pull Request

Description

Closes #4697. The example given there now produces

[Cell(point=cftime.datetime(2022, 3, 8, 12, 0, 0, 0, calendar='standard', has_year_zero=False), bound=None), Cell(point=cftime.datetime(2022, 3, 9, 12, 0, 0, 0, calendar='standard', has_year_zero=False), bound=None)]

The first workaround was added at #1016. From the comments on the tests I removed, the comparisons were then falling back to comparing object ids, so presumably rich comparison was not implemented in that version of netcdftime at all. It is implemented now:
https://github.com/Unidata/cftime/blob/dc75368cd02bbcd1352dbecfef10404a58683f94/src/cftime/_cftime.pyx#L1314

The second workaround was added at #2440. The comment there says that NotImplemented was not always being returned by the comparison method. If I've understood correctly, @bjlittle put in a fix for this at Unidata/cftime#81 where, as far as I can tell, python2 was the main complication. @bjlittle's fix was merged in 2018 and we are now pinned to cftime >= 1.5 which is only a year old. So I think we should be safe.

I'm currently targetting main, but I wonder if this should actually go in a patch release? #4697 suggests that this has now become a problem because time cell points have changed from being datetime.datetime to cftime.datetime. This was a breaking change in cf-units v3.0.


Consult Iris pull request check list

@trexfeathers trexfeathers self-assigned this May 5, 2022
@trexfeathers
Copy link
Contributor

@pdearnshaw we're not planning a bugfix release for this, so it would instead be released as part of Iris v3.3. Is that OK with you or is this a particularly urgent issue?

Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

@rcomer What's New and I'll merge 👍

@trexfeathers trexfeathers merged commit d1f3e45 into SciTools:main Oct 7, 2022
@rcomer
Copy link
Member Author

rcomer commented Oct 7, 2022

Thanks @trexfeathers!

@rcomer rcomer deleted the cell-datetime-comparisons branch October 7, 2022 10:58
@pp-mo pp-mo mentioned this pull request Mar 31, 2023
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.

Problems comparing time coord cell objects
2 participants