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

Loss of 'normal numbers' for times cells is limiting #2953

Closed
pp-mo opened this issue Feb 13, 2018 · 6 comments
Closed

Loss of 'normal numbers' for times cells is limiting #2953

pp-mo opened this issue Feb 13, 2018 · 6 comments
Assignees

Comments

@pp-mo
Copy link
Member

pp-mo commented Feb 13, 2018

We've now deprecated the use of Iris.FUTURE.cell_datetime_objects = False
(in Iris 2 rc0).
But, when data has non-standard calendars, this is a pain because then comparison between cell points does not work.

This can now get rather ugly (see below).
It also seems to me that the use of cell datetimes is clearly a special case, and maybe shouldn't be the default (let alone the only) behaviour
So, Should we drop this deprecation ?

Example : real user case :
Want to discard the first 3 timepoints of a cube.

WAS:

time_pts = cube.coord('time').points[3:]
new_cube = cube.extract(time=time_pts)

OR (probably better)

t1 = cube.coord('time').points[2]
new_cube = cube.extract(time=lambda t: t >= t1)

But these methods no longer work.
You need to explicitly "convert to numbers", like this :

time_coord = cube.coord('time') 
t1 = time_coord.points[2]
time_units = time_coord.units
new_cube = cube.extract(time=lambda t; time_units.date2num(t) >= t1)

Or use slicing instead :

time_dim, = cube.coord_dims('time')
slices = [slice(None)] * cube.ndim
slices[time_dim] = slice(3, None)
new_cube = cube[tuple(slices)]

ugh !!

@pelson
Copy link
Member

pelson commented Feb 14, 2018

Yep, this is an implication of using datetimes as cell values. It would be nice if we could use a coordinate in a constraint...

cube.extract(time=cube.coord('time')[:3])

I don't think this works though.

The proposal in #1988 would give us isel to do precisely this. (thought I don't think we are likely to move forwards with that any time soon).

Your 2 liner:

time_pts = cube.coord('time').points[3:]
new_cube = cube.extract(time=time_pts)

could be re-written as (untested)

time_pts = [cube.coord('time').cell(i).point for i in range(3)]
new_cube = cube.extract(time=time_pts)

Should we drop this deprecation

The easy answer to this is - absolutely not. Behaviour change always has implications, and this was a known implication of moving from a naive numeric representation to a richer datetime one.

Happy to close this @pp-mo?

@rcomer
Copy link
Member

rcomer commented Feb 14, 2018

How about

cube.subset(cube.coord('time')[3:])

@rcomer
Copy link
Member

rcomer commented Feb 14, 2018

Or

t1 = cube.coord('time').cell(2)
new_cube = cube.extract(iris.Constraint(time=lambda t: t > t1))

(works in Iris1.13 with Iris.FUTURE.cell_datetime_objects = True)

@pp-mo
Copy link
Member Author

pp-mo commented Feb 14, 2018

How about ... cube.subset ?

Subset I hadn't thought of + looks like a really neat approach to this specific requirement.

or ... Constraint(time=lambda t: t > t1)

I think this is exactly the case that doesn't work : it works if you have a standard calendar (hence "real" datetimes), but not for e.g. a model calendar : the cell points are then netcdf-datetimes, which don't support comparison operations.
It's exactly that, that makes me question whether the "future" is really a forward step here.

@pp-mo
Copy link
Member Author

pp-mo commented Feb 14, 2018

Alternative methods aside, my original point is that the new way of treating coordinate values as datetimes is rather awkward in these cases, as (some) datetimes just don't provide all the behaviours you need + expect.

Should we drop this deprecation ... absolutely not. Behaviour change always has implications, and this was a known implication of moving from a naive numeric representation to a richer datetime one.

As it is not a "richer" tool in this case, I still wonder whether the 'new' behaviour is actually a forward step.
Obviously having the choice was an improvement, but it's not so clear that datetimes-only is.

Or, is there a case that there is "always" a good way of doing these things in the new regime ?

@pelson
Copy link
Member

pelson commented Feb 14, 2018

as (some) datetimes just don't provide all the behaviours you need + expect.

I'm pretty sure that has now changed:

>>> netcdftime.Datetime360Day(2000, 2, 29) > netcdftime.Datetime360Day(2000, 2, 30)
False
>>> netcdftime.Datetime360Day(2000, 2, 29) < netcdftime.Datetime360Day(2000, 2, 30)
True
>>> netcdftime.__version__
'1.4.1'

To reiterate my statement:

Behaviour change always has implications

We knew what we were getting into with this change. I don't think there is anything here that makes me question that decision.

I'm going to close this one out. Conversation is welcome to continue (and even re-open if we get new information that is likely to make us change that decision).

@pelson pelson closed this as completed Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants