Skip to content

Fix bug in Date.to_timedelta - #85

Merged
pwolfram merged 2 commits into
MPAS-Dev:developfrom
xylar:fix_Date_timedelta_bug
Jan 30, 2017
Merged

Fix bug in Date.to_timedelta #85
pwolfram merged 2 commits into
MPAS-Dev:developfrom
xylar:fix_Date_timedelta_bug

Conversation

@xylar

@xylar xylar commented Jan 27, 2017

Copy link
Copy Markdown
Collaborator

This merge fixes a bug in Date.to_timedelta where the local variable day is used instead of self.day.

Also, fix some PEP8 issues in Date.

Also, fix some PEP8 issues in Date
@xylar

xylar commented Jan 27, 2017

Copy link
Copy Markdown
Collaborator Author

No existing code (except our CI tests) uses Date.to_timedelta so analysis should be unaffected by this bug.

My tests on an ACME G run with MPAs at QU240km resolution show that the results are bit-for-bit unchanged. (I did not test sea ice climatology plots because I don't have the mapping files for this particular grid.)

@xylar

xylar commented Jan 27, 2017

Copy link
Copy Markdown
Collaborator Author

@milenaveneziani, this should hopefully be an easy review. I may update the CI test because it somehow didn't catch this bug so probably could use a more rigorous test.

@xylar

xylar commented Jan 27, 2017

Copy link
Copy Markdown
Collaborator Author

I added the more rigorous test.

@@ -117,7 +118,7 @@ def to_timedelta(self):
"instead of to_timedelta")

days = 365*self.years + self._monthsToDays(self.months) + self.days

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assumes that there are no leap years, which is not general and probably needs fixed in #82 for the case where leap years may be allowed for MPAS.

@pwolfram

Copy link
Copy Markdown
Contributor

@xylar, regarding

No existing code (except our CI tests) uses Date.to_timedelta so analysis should be unaffected by this bug.

this doesn't seem to be the case, e.g., https://github.com/MPAS-Dev/MPAS-Analysis/blob/develop/mpas_analysis/shared/mpas_xarray/mpas_xarray.py#L189. This could have unexpected side effects for the case of 'timedelta64[ns]'-based dataset time dimensions, which are not being tested via the QU240km. However, in all fairness this is clearly not the 85% of cases we routinely consider.

@xylar

xylar commented Jan 28, 2017

Copy link
Copy Markdown
Collaborator Author

@pwolfram, I think you're maybe confusing Date.to_timedelta with pandas.to_timedelta, which are not related functions (other than that both produce a timedelta object). mpas_xarray is fully unaware of the existence of the Date class, so this change can't affect it. Grepping through the code in the current develop, I see:

$ grep -r "to_timedelta" .
./mpas_analysis/shared/timekeeping/Date.py:            raise ValueError("self.isInterval == True. Use to_timedelta "
./mpas_analysis/shared/timekeeping/Date.py:    def to_timedelta(self):
./mpas_analysis/shared/timekeeping/Date.py:                             "instead of to_timedelta")
./mpas_analysis/shared/mpas_xarray/mpas_xarray.py:                     pd.to_timedelta(time_var.values, unit='ns')]
./mpas_analysis/test/test_date.py:        timedelta1 = date.to_timedelta()

The only call to Date.to_timedelta is in the testing infrastructure.

@pwolfram

Copy link
Copy Markdown
Contributor

@xylar, thanks for clarifying.

@pwolfram
pwolfram merged commit 4527c9c into MPAS-Dev:develop Jan 30, 2017
@xylar

xylar commented Jan 30, 2017

Copy link
Copy Markdown
Collaborator Author

Thanks @pwolfram!

@xylar
xylar deleted the fix_Date_timedelta_bug branch February 1, 2017 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants