Design Document: reorganizing timekeeping #103

xylar · 2017-02-06T14:17:14Z

A design document for reorganizing the timekeeping module.

milenaveneziani · 2017-02-07T04:30:13Z

design_docs/timekeeping_reorg.md

+date: 2017/02/06 <br>
+</h1>
+<h2> Summary </h2>
+Currently, a the `Date` class is used to parse a date object from a date string


small typo: a the

milenaveneziani · 2017-02-07T04:33:26Z

design_docs/timekeeping_reorg.md

+date formats: date strings, days since a reference date, `datetime.datetime` objects
+and `relativedelta` objects (see below).  The success of this reorganization will be
+demonstrated when the existing analysis can be performed successfully with the new
+utility functions with both the `'gregorian_noleap'` calendar used by most existing


Does it make sense to call the noleap calendar 'gregorian_noleap'? I would personally be in favor of 'gregorian' versus simply 'noleap' (or no_leap, or perhaps '365_noleap').

@milenaveneziani, these are the names used in MPAS cores. I am sticking with those names unless there's a compelling reason to change.

milenaveneziani · 2017-02-07T04:35:25Z

design_docs/timekeeping_reorg.md

+</h2>
+
+There must be a way to parse dates from MPAS that is aware of the appropriate calendar to
+use, either `'gregorian'` or `'gregorian_noleap'`.


By this you mean that there should be a CF compliant time variable in MPAS files that tells us what calendar was used?

No, I mean there should be a way to use the namelist option config_calendar_type to determine which calendar is correct, and then to parse date strings accordingly. I will clarify.

milenaveneziani · 2017-02-07T04:37:28Z

design_docs/timekeeping_reorg.md

+
+As long as `relativedelta` objects rather than `datetime.timedelta` objects are used to increment
+`datetime.datetime` objects, `datetime.datetime` can be used to represent dates on either the Gregorian
+or the 365-day clanedars.


small typo here.

milenaveneziani · 2017-02-07T04:51:17Z

design_docs/timekeeping_reorg.md

+</h2>
+Analysis will be run on Edison with all available configurations found in `configs/edison`.  In addion,
+a test ACME run will be performed on LANL IC with the `gregorian` calendar option, and this will be
+used to test the analysis with the alternative calendar.


I do not think we currently have an MPAS or ACME run that uses the gregorian calendar. I am pretty sure that for now we are only using the 365_noleap.
We should find out what is the plan for future simulations.

A google search revealed that CESM 1.2 supports both noleap and gregorian calendars:
CESM supports a 365 day (or no-leap) calendar as well as a gregorian calendar. The calendar is set by the xml variable, CALENDAR, in env_build.xml. The no-leap calendar has the standard 12 months, but it has 365 days every year and 28 days in every February. Monthly averages in CESM are truly computed over varying number of days depending on the month of the year. In CESM1.0.x, a gregorian calendar was only possible if the ESMF library was used. This is no longer the case in CESM1.1.x and CESM1.2.x.

But I suspect that in our previous CESM-based simulations we always used noleap.

@milenaveneziani, that is why I was proposing to perform a new ACME simulation with gregorain.

I see, sorry for not reading your text more carefully.
I didn't know that we already have those two calendar options in MPAS (I've been meaning to look at the code today, but seems all LCFs as well as LANL IC have conjured on system maintenance for this day..).
It will be interesting to see how things go when choosing the gregorian calendar.

@xylar: I talked with a few people about this and also brought up the subject at the MPAS meeting today, and, contrary to what I thought initially, there is a general agreement that we are not going to use the gregorian calendar in MPAS or ACME simulations. Even for transient simulations, CESM has always used the noleap calendar (the transient forcing just does not have anything for the Feb 29's). So, I would say it is good if we support both (as MPAS and ACME/CESM do), but we probably do not want to invest time on running additional testing.

Okay, great! That saves me some time. I'll update the testing plan accordingly.

pwolfram · 2017-02-08T21:40:25Z

design_docs/timekeeping_reorg.md

+</h2>
+Analysis will be run on Edison with all available configurations found in `configs/edison`.  As there
+are currently no plans to run with the `gregorian` calendar option, we do not have test runs that use this
+calendar.  If this situation changes in the future, we'll test at that time.


Shouldn't we still test functionality via py.test, even if it isn't fully in use? Otherwise, we could argue we don't need to fully flesh out the option that is neither tested nor used.

Yes, and I'm doing that although I don't have any tests that specifically test the gregorian vs. the gregorian_noleap calendars. Do you have a suggestion? Maybe I should try to add one day to 2016-02-28 and see what I get?

pwolfram · 2017-02-08T21:40:58Z

design_docs/timekeeping_reorg.md

+calendar.  If this situation changes in the future, we'll test at that time.
+
+Regression tests previously for `Date` has been modified to test the new utility functions.
+


I'm confused, are we or are not testing the gregorian calendar option?

No, we are not, because we are not likely to use it in ACME or MPAS in the near (or not-so-near) future. See my last comment after you click on View changes (or however you do to pull out previous review comments..).

pwolfram · 2017-02-08T21:43:54Z

@xylar, this looks good to me except for my concern about not testing implemented functionality.

xylar · 2017-02-08T22:14:15Z

@pwolfram, I have tested both calendars in the CI but I don't have a run with gregorian and there is no plan to have one in the near future, as we discussed a bit ago. I will add a CI test that specifically tries to make sure the calendars are the ones we expect: for gregorian, 2016-02-28 + 1 day should be 2016-02-29 but for gregorian_noleap it should be 2016-03-01.

xylar · 2017-02-08T22:48:15Z

@pwolfram, damn! Good thing you made me add more testing! relativedelta is surprisingly buggy. I don't think we're going to be able to use it after all. I'll submit some bug reports, but I'm guessing the turnaround won't be fast enough to be helpful for us.

I'll play with it some more tomorrow.

xylar · 2017-02-09T14:14:07Z

After adding CI, I discovered that relativedelta did not work as expected for the gregorain_noleap calendar. After tinkering a bit, it became clear that the existing functionality would not cover our needs entirely so I created a subclass of relativedelta (MpasRelativeDelta) to handle addition (and subtraction, multiplication, etc.) that works correctly with both calendars.

@pwolfram, I realize you prefer that we use existing libraries wherever possible but I hope this compromise (building on top of an existing library but adding functionality to address our needs) works for you.

pwolfram · 2017-02-09T16:28:28Z

@xylar, this type of thing will be unavoidable given the absolute and persistent confusion regarding how dates should be handled through and across multiple libraries. I fully agree a solution, as long as it is as compact as possible, is what we need even if it strictly isn't clean. There really don't appear to be any clean solutions in this space and top experts in this area essentially say things along the lines of "extensive numpy / scipy / pandas / xarray refractoring and development is needed", which implies we need some type of custom solution like you are proposing.

pwolfram · 2017-02-09T16:29:58Z

I should also note that we would have eventually run into calendar problems ourselves as pointed out in the MPAS-O meeting last week by @maltrud, even if we restricted ourselves to the minimal matlab-equivalent python stack of numpy / matplotlib.

xylar · 2017-02-10T13:26:47Z

I have squashed commits and I think this is ready to merge. @milenaveneziani, would you do the honors?

pwolfram

LGTM, I'm happy to merge this before / after #102 although technically I think we should merge the design docs before the code that implements them.

xylar · 2017-02-10T17:04:08Z

Thanks @milenaveneziani

@pwolfram wrote:

technically I think we should merge the design docs before the code that implements them.

I suppose that's true. My thinking was that you might not be sure you're done making changes to the design document (e.g. details of testing or implementation) until you've merged the branch. But essentially both should happen simultaneously.

xylar added the design document label Feb 6, 2017

xylar assigned pwolfram and milenaveneziani Feb 6, 2017

xylar mentioned this pull request Feb 6, 2017

A reorganization of timekeeping to work with general calendars #102

Merged

milenaveneziani reviewed Feb 7, 2017

View reviewed changes

pwolfram reviewed Feb 8, 2017

View reviewed changes

xylar added the in progress label Feb 10, 2017

Add a design doc for timekeeping reorganization

8a38f08

xylar force-pushed the design_doc_timekeeping_reorg branch from bf02909 to 8a38f08 Compare February 10, 2017 13:26

xylar removed the in progress label Feb 10, 2017

pwolfram approved these changes Feb 10, 2017

View reviewed changes

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

xylar deleted the design_doc_timekeeping_reorg branch February 10, 2017 17:02

		calendar. If this situation changes in the future, we'll test at that time.

		Regression tests previously for `Date` has been modified to test the new utility functions.

Design Document: reorganizing timekeeping #103

Design Document: reorganizing timekeeping #103

Uh oh!

Conversation

xylar commented Feb 6, 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!

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

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!

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

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pwolfram commented Feb 8, 2017

Uh oh!

xylar commented Feb 8, 2017

Uh oh!

xylar commented Feb 8, 2017

Uh oh!

xylar commented Feb 9, 2017

Uh oh!

pwolfram commented Feb 9, 2017

Uh oh!

pwolfram commented Feb 9, 2017

Uh oh!

xylar commented Feb 10, 2017

Uh oh!

pwolfram left a comment

Choose a reason for hiding this comment

Uh oh!

xylar commented Feb 10, 2017

Uh oh!

Uh oh!

milenaveneziani Feb 7, 2017 •

edited

Loading

milenaveneziani Feb 8, 2017 •

edited

Loading

milenaveneziani Feb 8, 2017 •

edited

Loading