fix for issue #198 (take 3) #202
Conversation
|
I like this solution! It looks like it shouldn't break existing user |
|
@spencerkclark thanks for offering to review - I think it is ready |
There was a problem hiding this comment.
This looks great @jswhit. I mostly have just cosmetic comments. One general question -- is there a strong reason why the default value for the calendar argument in the cftime.datetime constructor is 'standard'? As I understand it, we could make things completely backwards compatible if it was ''.
| #def to_calendar_specific_datetime(dt, calendar, use_python_datetime): | ||
| # if use_python_datetime: | ||
| # return real_datetime( | ||
| # dt.year, | ||
| # dt.month, | ||
| # dt.day, | ||
| # dt.hour, | ||
| # dt.minute, | ||
| # dt.second, | ||
| # dt.microsecond) | ||
| # else: | ||
| # return datetime( | ||
| # dt.year, | ||
| # dt.month, | ||
| # dt.day, | ||
| # dt.hour, | ||
| # dt.minute, | ||
| # dt.second, | ||
| # dt.microsecond, | ||
| # calendar=calendar) |
There was a problem hiding this comment.
Should this be deleted now?
There was a problem hiding this comment.
The commented out version does not return the calendar-specific subclasses. Since we might want to remove those sub-classes in a future release, I left it in there.
There was a problem hiding this comment.
Gotcha -- it makes sense to leave it in then.
test/test_cftime.py
Outdated
| @@ -1554,7 +1520,9 @@ def test_num2date_only_use_cftime_datetimes_negative_years( | |||
| calendar, expected_date_type): | |||
| result = num2date(-1000., units='days since 0001-01-01', calendar=calendar, | |||
| only_use_cftime_datetimes=True) | |||
| assert isinstance(result, expected_date_type) | |||
| #assert isinstance(result, expected_date_type) | |||
There was a problem hiding this comment.
| #assert isinstance(result, expected_date_type) |
test/test_cftime.py
Outdated
| @@ -1565,7 +1533,9 @@ def test_num2date_only_use_cftime_datetimes_pre_gregorian( | |||
| calendar, expected_date_type): | |||
| result = num2date(1., units='days since 0001-01-01', calendar=calendar, | |||
| only_use_cftime_datetimes=True) | |||
| assert isinstance(result, expected_date_type) | |||
| #assert isinstance(result, expected_date_type) | |||
There was a problem hiding this comment.
| #assert isinstance(result, expected_date_type) |
| # return calendar-specific subclasses for backward compatbility, | ||
| # even though after 1.3.0 this is no longer necessary. | ||
| if calendar == '360_day': | ||
| #return dt.__class__(*add_timedelta_360_day(dt, delta),calendar=calendar) |
There was a problem hiding this comment.
Are these commented lines relevant?
There was a problem hiding this comment.
I left those there in case we decide to remove the calendar-specific subclasses. They are really not needed, but I use them here just in case some is expected them to be returned by __add__ and __sub__ (since that was the previous behavior).
cftime/_cftime.pyx
Outdated
| self.datetime_compatible = True | ||
| assert_valid_date(self, is_leap_proleptic_gregorian, False) | ||
| self.has_year_zero = False | ||
| elif calendar == '': |
There was a problem hiding this comment.
Is there a reason '' was chosen instead of None?
There was a problem hiding this comment.
Probably, but the reason has been lost in the fog of history. I think @davidhassell may have suggested this.
The default has always been 'standard', but before this didn't actually create a calendar-aware instance. |
Right, I guess the question is: do we prefer to preserve the way the The advantage of the former is that it would not require any changes in nc-time-axis or xarray. The advantage of the latter is that it might be friendlier default behavior to return a calendar-aware instance. The xarray changes are trivial -- we just need to modify this test, which honestly probably should be modified anyway. If that was all that was required, I'd be inclined to go with the user-friendlier option. I'm a little more concerned about nc-time-axis, since they still have this 1It may be that things still work most of the time even if there is a mismatch between the calendar type of the instance and the calendar specified in the |
I'm happy either way. I'm not sure what the use case is for creating a 'dumb' (calendar-naive) instance though. |
|
I went ahead and changed the default calendar to None, which behaves the same as '' (and creates a calendar-naive instance). The |
|
Based on the discussion in issue #173, I got nervous about the consequences of changing the default calendar (so I changed it back to 'standard'). However, the default behaviour will still change, in that the datetime instance will now be calendar aware by default. @davidhassell - do you foresee any issues with this change? |
There was a problem hiding this comment.
Sounds good @jswhit; I'll take care of the xarray change. It looks like you've already made a start on the nc-time-axis changes, which is great. Hopefully the developers there will be able to provide guidance on how they'd like to adapt to this update.
This approach removes the use of sub-classes to create 'calendar-aware' cftime.datetime instances. Instead, the cftime.datetime base class uses the calendar kwarg to create 'calendar-aware' instances. The sub-classes are no longer used internally, but retained for backwards compatibility. The disadvantage of this approach is that user code which uses issubclass may break.