A reorganization of timekeeping to work with general calendars #102

xylar · 2017-02-06T14:13:12Z

The Date class has been replaced by a few utility functions:

stringToDatetime - converts an MPAS date string to a datetime.datetime
stringToRelativeDelta - converts an MPAS date string to an MpasRelativeDelta
clampToNumpyDatetime64 - clamps a datetime to the valid range supported by xarray

A new MpasRelativeDelta class has been added (deriving from dateutil.relativedelta.relativedelta)

All analysis functions and tests have been updated to work with these utility functions.

xylar · 2017-02-06T14:13:44Z

This branch needs to be rebased after #86 gets merged.

xylar · 2017-02-06T14:15:03Z

Testing

I tested on my laptop using a G-240km test case (and skipping regriddedSeaIceConcThick because, as usual, I don't have the mapping file).

I ran the test cases pointed to by both config.20170120.beta0.GMPAS-QU240.wolf and config.20161117.beta0.A_WCYCL1850S.ne30_oEC_ICG.edison successfully on wolf.

xylar · 2017-02-06T14:18:13Z

This branch implements the design in #103

pwolfram · 2017-02-08T21:45:05Z

@xylar, I'll be happy to do a review following the rebase to at least give some comments.

xylar · 2017-02-09T14:19:43Z

@pwolfram, when you get around to reviewing this PR, please take a look at the CI I added for testing my new MpasRelativeDelta class near leap days. Feel free to suggest other tests I should add if you can think of any.

pwolfram · 2017-02-09T16:38:20Z

mpas_analysis/shared/timekeeping/MpasRelativeDelta.py

+        return self.__neg__().__radd__(other)
+
+    def __sub__(self, other):
+        return NotImplemented


Is there a reason why subtraction isn't supported but addition is supported? You also have negation, which suggests subtraction should work as a combination of addition and negation (or multiplication with a negative factor), e.g., A - B = A + (-B). This should be commented or implemented.

So that's because date - offset is sensible but offset - date doesn't really make sense. So __rsub__ is okay but __sub__ should be undefined. Have I missed something? Keep in mind that addition and subtraction is with datetime.datetime objects, not with other MpasRelativeDelta objects.

I would argue that the subtraction operation needs to be commutative so that offset - date should make sense because it is just offset - date = - (date - offset). This is because the negation operator is defined and I'm using standard arithmetic set operations, which I think make sense in this context and is presumed by many. Alternatively, we should issue a warning that this is not implemented, which is true in general for operators like this that seem like they should intuitively be implemented.

Negation operator on a date doesn't make sense and puts a date out of range. So I don't think either offset - date or - (date - offset) are supported. I don't believe datetime.datetime supports negation, and I think that is correct.

So I would say that offset1 - offset2 = - (offset2 - offset1) if you're using two MpasRelativeDeltas but if you're using datetime.datetime as one of the objects, that's no longer necessarily a sensible operator.

pwolfram · 2017-02-09T16:39:37Z

mpas_analysis/shared/timekeeping/MpasRelativeDelta.py

+    Only relative intervals (years, months, etc.) are supported and not the
+    absolute date specifications (year, month, etc.).  Addition/subtraction
+    of datetime.datetime objects (but not other MpasRelativeDelta,
+    datetime.timedelta or other related objects) is supported.


Why is this the case? Additional comments are needed or this restriction should be removed.

I don't think we have a use case where we need to be able to add or subtract two offsets. I'm not sure if it's allowed for timedeltas. I thought not but I'll check.

Okay, it looks like timedelta supports addition and subtraction of other timedeltas so I'll implement it. It doesn't seem that hard.

We should test all overloaded operator operations too because this is best practices and will ensure that we get what is expected.

I believe I'm testing them all at this point.

pwolfram · 2017-02-09T16:40:49Z

mpas_analysis/shared/timekeeping/utility.py

+    """
+    Temporary function for adding an offset year and clamping a datetime to
+    range supported by `numpy.datetime64`.
+    """


Should this issue a warning (perhaps that can be turned off via a flag) that specifies which direction the clamp, i.e., floor or ceil, was performed?

I see, I had misread your comment the first time. There are a lot of cases where this function is used precisely because the clamping behavior is desired, so it would be important to suppress the warning in those cases. I'm inclined not to add the complexity required to have an optional warning, given that this function really is temporary and the next PR I am planning to make will eliminate this function entirely, along with yearOffset. But I can add this functionality if you feel strongly about it.

If we are going to get rid of this on a short timescale of weeks, but not months, this is noise and we should proceed in the less rigorous way.

It's my intention to have this replaced as soon as possible (definitely weeks rather than months). That partly depends on the speed of reviews and testing of the PRs that come before it. So I appreciate you working on this.

pwolfram · 2017-02-09T16:41:56Z

mpas_analysis/test/test_timekeeping.py

+        self.assertEqual(date1, date2)
+
+        # gregorian
+        calendar = 'gregorian'


How about looping over gregorian and gregorian_noleap?

Yep, I'll do that.

We want to test the whole range of acceptable dates for both and then the distinction between gregorian and gregorian_noleap for each distinguishing case, e.g., Feb 29.

Really? We can do that, but that seems pretty extreme to me.

I guess I don't really understand what you mean by "test the whole range of acceptable dates"? I guess you mean the different string formattings? I think it's pretty redundant to check this for both calendars because the function for parsing these strings is completely independent of the calendar, it just keeps the calendar around for doing math operations later on. So in any cases where we're not doing math, the calendar is irrelevant.

But I'm looping over both calendars anyway. So I guess this is resolved.

pwolfram · 2017-02-09T16:42:43Z

mpas_analysis/test/test_timekeeping.py

+        date2 = datetime.datetime(year=2262, month=1, day=1)
+        self.assertEqual(date1, date2)
+
+        # test if the calendars behave as they should


The loop could go here and reduce the number of lines of code required by about a factor of two.

I don't mind doing this and I agree it makes sense to systematically test both calendars but there will be no large reduction in code. Each test is unique (except for the first 2) independent of calendar.

Right, this makes sense and will clarify the code albeit not at a huge reduction in the amount of code. It will be clearer that this set of cases responds the same for each calendar and this set responds differently. Perhaps you could do something like I'll show you below in another comment... to make the distinction clearer.

Okay, I think this is addressed.

pwolfram · 2017-02-09T16:43:24Z

mpas_analysis/test/test_timekeeping.py

+                                      calendar='gregorian')
+        diff = date1-date2
+        self.assertEqual(diff, stringToDatetime('1995-12-26'))
+


I would like to see some tests on operators for MpasRelativeDelta instances.

Many of these tests already do test addition and subtraction operators. I will add some more systematic tests of all operators, though.

This would be good. But you could also note that a particular test is doing both in a comment too if that is cleaner.

Yep, I've done so.

pwolfram · 2017-02-09T16:44:08Z

@xylar, I added some incomplete and preliminary comments on this PR to get you started.

pwolfram

I've added some preliminary comments related to time-based classes and testing. I suspect there may be some more comments following the rebase and further understanding of additional challenges with calendars.

pwolfram · 2017-02-09T18:59:01Z

mpas_analysis/test/test_timekeeping.py

+        self.assertEqual(stringToDatetime('2016-03-01') -
+                         stringToRelativedelta('0000-00-01',
+                                               calendar='gregorian'),
+                         stringToDatetime('2016-02-29'))


How about simplifying via something like:

for calendar, expected in zip(['gregorian', 'gregorian_noleap'], ['2016-03-01', '2016-02-29']): self.assertEqual(stringToDateTime('2016-03-01') - stringToRelativedelta('0000-00-01', calendar=calendar), stringToDateTime(expected))

This more clearly shows the distinction between the calendars and their usage and reduces the amount of needed code.

Okay, I'll do that.

xylar · 2017-02-09T19:57:49Z

@pwolfram, I think I've addressed all your comments for now. Why don't you wait until we can merge #86 and I can rebase before continuing your review?

xylar · 2017-02-10T07:25:27Z

@pwolfram and @milenaveneziani, this PR has been rebased onto develop and is ready to review. I have left on the "Don't Merge" flag because I want to squash some commits before it gets merged.

Remove Date class. Update streams to use new timekeeping utility. Update all analysis scripts to use timekeeping utility. Update testing to new timekeeping utility funs.

xylar · 2017-02-10T13:22:16Z

Okay, I have tested this PR on Edison (while /project is still available!) with the usual beta0 run (in configs/edison) and results are bit-for-bit to those from develop.

I squashed commits, so the PR is now ready to merge.

@pwolfram, please let me know if there are remaining concerns that I haven't addressed.

@milenaveneziani, please test and merge once you get the go-ahead from @pwolfram.

pwolfram

I have a few extremely minor comments and this is great forward progress and I'm signing-off-- LGTM.

pwolfram · 2017-02-10T15:02:48Z

mpas_analysis/shared/timekeeping/MpasRelativeDelta.py

+    __truediv__ = __div__
+
+    def __repr__(self):
+        l = []


This is dangerous for multiple reasons-- I would greatly prefer that we never use single-character variables unless they are an index. Furthermore, "l" looks a lot like a "1", especially in certain fonts. Cna you please make this something like rep or strrep, e.g. something three characters or more instead of l?

Sorry, I copied this code straight out of relativedelta and only modified the minimum to add the calendar. I agree that it's badly written. I'll clean it up.

pwolfram · 2017-02-10T15:05:50Z

mpas_analysis/shared/timekeeping/utility.py

+    Last modified
+    -------------
+    02/04/2017
+    """


An aside-- we should set something up that produces a "readthedocs" descrition of at least our public API. Feel free to do the honors for an issue or I can also do so. I'm happy to set this up when I get a a moment too if you would like so that we can leverage all your really good work getting docstrings into a more general sphinx-compatable format.

Go for it. I agree that this needs to happen but I haven't had time (and won't for awhile) to look into it further.

pwolfram · 2017-02-10T15:07:20Z

mpas_analysis/test/test_timekeeping.py

+        # YYYY-MM-DD
+        # SSSSS
+
+        for calendar in ['gregorian', 'gregorian_noleap']:


Great! Thanks @xylar :-)

pwolfram · 2017-02-10T15:07:41Z

mpas_analysis/test/test_timekeeping.py

+        date2 = datetime.datetime(year=2262, month=1, day=1)
+        self.assertEqual(date1, date2)
+
+    def test_MpasRelativeDeltaOps(self):


pwolfram · 2017-02-10T15:08:02Z

mpas_analysis/test/test_timekeeping.py

+
+        # make sure there's an error when we try to add MpasRelativeDeltas
+        # with different calendars
+        with self.assertRaisesRegexp(ValueError,


Perfect, because this enforces a design intent.

This merge adds a new MpasRelativeDelta class derived from dateutils.relativedelta.relativedelta. This class extends relativedelta to handle both MPAS calendars (important for addition to/subtraction from datetime.datetime objects). The timekeeping tests have been updated to work with the new class and to include new tests that ensure the right behavior for manipulating dates near leap days.

xylar · 2017-02-10T15:27:53Z

@pwolfram, I think I've addressed your remaining comment.

pwolfram · 2017-02-10T16:59:50Z

@xylar, I'm giving the go-ahead to @milenaveneziani for the merge.

milenaveneziani · 2017-02-10T17:00:37Z

I tested this on wolf using the GMPAS-240 case and including the sea-ice climos. All went well.
Here is the sea-ice concentration for JFM, for example:

xylar added clean up in progress labels Feb 6, 2017

xylar assigned pwolfram and milenaveneziani Feb 6, 2017

xylar mentioned this pull request Feb 8, 2017

Move variable mapping out of mpas_xarray into new generalized_reader module #104

Merged

pwolfram reviewed Feb 9, 2017

View reviewed changes

pwolfram suggested changes Feb 9, 2017

View reviewed changes

pwolfram reviewed Feb 9, 2017

View reviewed changes

xylar force-pushed the timekeeping_reorg branch from c57ef11 to 31e8742 Compare February 9, 2017 19:53

xylar force-pushed the timekeeping_reorg branch from 31e8742 to 9178c1c Compare February 10, 2017 07:22

xylar force-pushed the timekeeping_reorg branch from 9178c1c to c22e6c8 Compare February 10, 2017 07:28

Add timekeeping utility functions

00a30d2

Remove Date class. Update streams to use new timekeeping utility. Update all analysis scripts to use timekeeping utility. Update testing to new timekeeping utility funs.

xylar force-pushed the timekeeping_reorg branch from c22e6c8 to a44e572 Compare February 10, 2017 13:19

xylar removed the in progress label Feb 10, 2017

pwolfram approved these changes Feb 10, 2017

View reviewed changes

xylar force-pushed the timekeeping_reorg branch from a44e572 to f5382cd Compare February 10, 2017 15:24

xylar mentioned this pull request Feb 10, 2017

Date class only supports 365 day calendar #99

Closed

milenaveneziani merged commit f5382cd into MPAS-Dev:develop Feb 10, 2017

xylar deleted the timekeeping_reorg branch February 10, 2017 17:04

xylar mentioned this pull request Feb 10, 2017

Design Document: Generalize Calendar supported by Analysis #105

Merged

A reorganization of timekeeping to work with general calendars #102

A reorganization of timekeeping to work with general calendars #102

Uh oh!

Conversation

xylar commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xylar commented Feb 6, 2017

Uh oh!

xylar commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

xylar commented Feb 6, 2017

Uh oh!

pwolfram commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xylar commented Feb 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pwolfram commented Feb 9, 2017

Uh oh!

pwolfram left a comment

Choose a reason for hiding this comment

xylar commented Feb 6, 2017 •

edited

Loading

xylar commented Feb 6, 2017 •

edited

Loading

pwolfram commented Feb 8, 2017 •

edited

Loading