Changes to raise exceptions when comparing bounded datetime cells #1016
Conversation
| @@ -184,14 +186,22 @@ def __common_cmp__(self, other, operator_method): | |||
| Non-Cell vs Cell comparison is used to define Constraint matching. | |||
|
|
|||
| """ | |||
| if not (isinstance(other, (int, float, np.number, Cell)) or | |||
| hasattr(other, 'timetuple')): | |||
There was a problem hiding this comment.
Is there a reason for the behaviour change? Doesn't this now prevent us comparing with normal datetimes?
There was a problem hiding this comment.
It does. I could revert this one or add datetime.datetime to the list. The danger of letting any timetuple possessing object through is that you could get datetime cells being compared to netcdftimes and getting nonsense.
There was a problem hiding this comment.
We could just do a not isinstance(other, netcdftime) along with the original check. I guess the question comes - do we want to be able to compare Cells with datetimes? In my head we do, but if there a reason that is a bad thing, then I wouldn't lose sleep over it....
|
Arrgh |
|
@pelson - I had a git hiccup which meant I accidentally rebased against upstream/master. I've cherry-picked my way out so hopefully that won't be a problem. travis is failing due to the netcdf issue that's been fixed on upstream/master but not in this branch. |
| @@ -281,6 +295,10 @@ def contains_point(self, point): | |||
| """ | |||
| if self.bound is None: | |||
| raise ValueError('Point cannot exist inside an unbounded cell.') | |||
| if hasattr(point, 'timetuple') or np.any([hasattr(val, 'timetuple') for | |||
| val in self.bound]): | |||
| raise TypeError('Cannot determine whether a point lies within ' | |||
There was a problem hiding this comment.
This is where having a PartialDateTime.within(lower_dt, upper_dt) would be valuable, right...
|
👍 - I think this is good to go. Any reason not to merge @esc24? |
|
Nope. I think it's good to go @pelson. How about I rebase just to let travis do its thing? |
Changes to raise exceptions when comparing bounded datetime cells
This PR is in response to unexpected and highly undesirable behaviour identified by #984. The effect of this PR is to raise an exception when attempting to constrain bounded time coords with the new date time cell comparison feature. This is disappointing, but without calendar information it is not possible to determine whether a PartialDateTime (or any other calendar-agnostic datetime-like object) lies within a bounded region. As Iris currently stands you'll get the wrong answer when performing such constraints and absolutely no indication that something has gone awry. Note that point based datetime constraints (i.e. those in the docs) are fine.
Please also notice that I've put in some special behaviour around
netcdftime.datetimeobjects as I can do:This fallback to id-based comparison (I think) makes comparing netcdftime.datetime objects to anything other than PartialDateTimes unreliable. I'd rather not put this time-specific/netcdftime-specific logic in
coords.pybut I can't think of a better place without a lot of work.