-
Notifications
You must be signed in to change notification settings - Fork 52
Fix time-series scripts use time bounds properly #48
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
""" | ||
|
||
import pytest | ||
import datetime | ||
from mpas_analysis.test import TestCase, loaddatadir | ||
from mpas_analysis.shared.timekeeping.Date import Date | ||
|
||
|
@@ -118,5 +119,37 @@ def test_date(self): | |
diff = date1-date2 | ||
self.assertEqual(diff, Date(dateString='1995-12-26', isInterval=False)) | ||
|
||
date = Date(dateString='1996-01-15', isInterval=False) | ||
datetime1 = date.to_datetime(yearOffset=0) | ||
datetime2 = datetime.datetime(year=1996, month=1, day=15) | ||
self.assertEqual(datetime1, datetime2) | ||
|
||
date = Date(dateString='0000-00-20', isInterval=True) | ||
timedelta1 = date.to_timedelta() | ||
timedelta2 = datetime.timedelta(days=20) | ||
self.assertEqual(timedelta1, timedelta2) | ||
|
||
# since pandas and xarray use the numpy type 'datetime[ns]`, which | ||
# has a limited range of dates, the date 0001-01-01 gets increased to | ||
# the minimum allowed year boundary, 1678-01-01 to avoid invalid | ||
# dates. | ||
date = Date(dateString='0001-01-01', isInterval=False) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would put a comment above this line reminding the reader that the date is specified based on the quasi-arbitrary xarray datetime boundary. |
||
datetime1 = date.to_datetime(yearOffset=0) | ||
datetime2 = datetime.datetime(year=1678, month=1, day=1) | ||
self.assertEqual(datetime1, datetime2) | ||
|
||
date = Date(dateString='0001-01-01', isInterval=False) | ||
datetime1 = date.to_datetime(yearOffset=1849) | ||
datetime2 = datetime.datetime(year=1850, month=1, day=1) | ||
self.assertEqual(datetime1, datetime2) | ||
|
||
# since pandas and xarray use the numpy type 'datetime[ns]`, which | ||
# has a limited range of dates, the date 9999-01-01 gets decreased to | ||
# the maximum allowed year boundary, 2262-01-01 to avoid invalid | ||
# dates. | ||
date = Date(dateString='9999-01-01', isInterval=False) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This tests the clipping capability, right? I think we should note this here and if there is a warning thrown we should make sure it is what we expect to get. |
||
datetime1 = date.to_datetime(yearOffset=0) | ||
datetime2 = datetime.datetime(year=2262, month=1, day=1) | ||
self.assertEqual(datetime1, datetime2) | ||
|
||
# vim: foldmethod=marker ai ts=4 sts=4 et sw=4 ft=python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we should issue a warning if we are on the clipping boundary so the unawares user will know there may be a potential problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree that this should be a warning. Typically want the end year of the time series analysis to be
9999
so we don't have to think about it. This is the function that makes sure that9999 + 1849
still gives you a reasonable year that the analysis can handle. If you didn't want the clamping and the year offset, you wouldn't bother calling this function.If I add a warning, it's going to be annoying that there are warnings throughout our output even though I'm intentionally using this function to clamp the date. The whole point of clamping the date here is so I don't have to do it elsewhere and so I can have better code reuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I see what you mean. We really aren't at a point where our simulations will go past 2262 anyway so I'm ok leaving this as-is for now. However, I'm not sure if we can necessarily plan on this for the future (in general) because I know Jeremy runs multi-century simulations. But I agree this is probably sufficient for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pwolfram, we don't support going past 100 years right now (because of limited numbers of files) so I think 2262 is the least of our concerns...
But also fixing
Date
with a new min/max allowed date will be super easy once xarray and pandas support a wider range. For now, we're at their mercy and this function is really only a side effect of that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Thanks for the reminder about the file limitation issue. I need to take a closer look at that once I get a chance.