Skip to content
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

Calendar conversion #426

Merged
merged 7 commits into from
Apr 30, 2020
Merged

Calendar conversion #426

merged 7 commits into from
Apr 30, 2020

Conversation

aulemahal
Copy link
Collaborator

@aulemahal aulemahal commented Apr 24, 2020

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes #xyz
  • Tests for the changes have been added (for bug fixes / features)
  • Documentation has been added / updated (for bug fixes / features)
  • HISTORY.rst has been updated (with summary of main changes)
  • bumpversion (minor / major / patch) has been called on this branch
  • Tags have been pushed (git push --tags)
  • What kind of change does this PR introduce?

Adds the new convert_calendar and interp_calendar methods (and some other utilities) to xclim.core.calendar to help in conversion of data between calendars.

convert_calendar simply modifies the type of the timestamps, not touching the data, unless invalid dates are found (like 29th of Feb on a "noleap" cal). In that case the timeslices (and data) are simply removed. This goes in the sense of #419 as it is completely agnostic of the data frequency.

interp_calendar interpolates the data while converting to the new calendar. While the other accepts named calendars, this one needs the target time coordinate. Interpolation is made on the "decimal year" basis, so the decimal number representing the date as a number of years since 1-01-01 AD. As such, it only gives something useful with daily data or coarser.

  • Does this PR introduce a breaking change?

Yes, I only kept references to calendars available in cftime (+ 'standard'). So I removed "no_leap" (use "noleap") and "uniform30day" (use "360_day").

  • Other information:

@aulemahal aulemahal added this to the v0.17 milestone Apr 24, 2020
@aulemahal aulemahal requested review from huard and tlogan2000 April 24, 2020 22:24
@aulemahal aulemahal self-assigned this Apr 24, 2020
@coveralls
Copy link

coveralls commented Apr 24, 2020

Pull Request Test Coverage Report for Build 1788

  • 80 of 86 (93.02%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 89.618%

Changes Missing Coverage Covered Lines Changed/Added Lines %
xclim/core/calendar.py 80 86 93.02%
Totals Coverage Status
Change from base Build 1766: 0.1%
Covered Lines: 2348
Relevant Lines: 2620

💛 - Coveralls

@aulemahal
Copy link
Collaborator Author

@Zeitsperre Working on the xarray failures this morning. Unless you already had some insight on them? Also any idea on the "connection failures" ones?

@aulemahal
Copy link
Collaborator Author

xarray@master is failing because of pydata/xarray#4009, not our fault!

@tlogan2000
Copy link
Collaborator

Should I start with this before sdba?

@aulemahal
Copy link
Collaborator Author

@tlogan2000 Yes! If this is ok, I would like to use it in sdba. Anyway, I don't think it be safe to merge sdba before the next release of xarray...

Copy link
Collaborator

@huard huard left a comment

Choose a reason for hiding this comment

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

I think this will need additional documentation to clarify the options to drop and add days.

xclim/core/calendar.py Show resolved Hide resolved
xclim/core/calendar.py Outdated Show resolved Hide resolved
Only converts the individual timestamps, does not modify any data excpet in dropping invalid/surplus dates.

If the source and target calendars are either no_leap, all_leap or standard, only the type of the time array is modified.
When converting to a leap year from a non-leap year, the 29th of february is simply removed from the array.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
When converting to a leap year from a non-leap year, the 29th of february is simply removed from the array.
When converting to a leap year from a non-leap year, the 29th of February is simply removed from the array.

xclim/core/calendar.py Outdated Show resolved Hide resolved
xclim/core/calendar.py Outdated Show resolved Hide resolved
xclim/core/calendar.py Outdated Show resolved Hide resolved
xclim/core/calendar.py Outdated Show resolved Hide resolved
xclim/core/calendar.py Show resolved Hide resolved
xclim/core/calendar.py Show resolved Hide resolved
xclim/core/calendar.py Show resolved Hide resolved
aulemahal and others added 2 commits April 27, 2020 13:22
Co-Authored-By: David Huard <huard.david@ouranos.ca>
@aulemahal
Copy link
Collaborator Author

aulemahal commented Apr 28, 2020

En fait, the '360_day' behaviour I coded might not be the best.
Question for @tlogan2000:
What would be the expected behaviour when converting a 360_day calendar to a standard one:

  1. Have the position of the data within a year stay approximately the same by a linear interpolation (the 5 or 6 missing point are now kinda "random" and different for leap and non-leap years).
  2. Have the dates stay the same, meaning all 31st will be NaN + 29th and 30th of Feb. So losing 1 or 2 datapoints (now missing 7 instead of 5 or 6).
  3. Have the dayofyear stay the same, having now the last week of the year missing.
  4. Have the user give a list of 6 missing days.

@tlogan2000
Copy link
Collaborator

En fait, the '360_day' behaviour I coded might not be the best.
Question for @tlogan2000:
What would be the expected behaviour when converting a 360_day calendar to a standard one:

  1. Have the position of the data within a year stay approximately the same by a linear interpolation
  2. Have the dates stay the same, meaning all 31st will be NaN + 29th and 30th of Feb. So losing 1 or 2 datapoints (now missing 7 instead of 5 or 6).
  3. Have the dayofyear stay the same, having now the last week of the year missing.

Definitely not option 3 I would think. Option 2 not great ... so I think I prefer option 1

Question though could nn interp be a better choice depending on variable?

Anyways I would go with linear for now and we can see if we add a nn interp as a user option at a later date

@tlogan2000
Copy link
Collaborator

En fait, the '360_day' behaviour I coded might not be the best.
Question for @tlogan2000:
What would be the expected behaviour when converting a 360_day calendar to a standard one:

  1. Have the position of the data within a year stay approximately the same by a linear interpolation (the 5 or 6 missing point are now kinda "random" and different for leap and non-leap years).
  2. Have the dates stay the same, meaning all 31st will be NaN + 29th and 30th of Feb. So losing 1 or 2 datapoints (now missing 7 instead of 5 or 6).
  3. Have the dayofyear stay the same, having now the last week of the year missing.
  4. Have the user give a list of 6 missing days.

Sorry I missed option 4. This seems ok I guess but should not be the default... Is this all coded already? Sorry haven't had time to look at the PR yet. Anyways, people already have trouble understanding the 360_day calendar so asking them to provide missing is likely to be an 'expert' only option

@aulemahal
Copy link
Collaborator Author

No, only 1 is coded. But to be clear : I am talking of interpolating the time coordinate only not the data. Right now, interp_calendar does that, reinterpolating datapoints on a new time coordinate aligning the data on its position within the year.
But convert_calendar doesn't touch the data (except for invalid dates that are dropped). Opt 1 is the one that realigns the data by interpolating the dayofyear attribute. Thus, the 5 or 6 missing datapoints. But that only makes sense daily (and subdaily) data.
On monthly data, that would mean that data points are not on the 1st of the month anymore... Opt 2 would not have that problem.

@aulemahal aulemahal requested a review from huard April 29, 2020 16:08
Copy link
Collaborator

@huard huard left a comment

Choose a reason for hiding this comment

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

Could only give a brief look, but nothing to comment on.

xclim/core/calendar.py Outdated Show resolved Hide resolved
@aulemahal aulemahal merged commit ab84bdc into master Apr 30, 2020
@aulemahal aulemahal deleted the easy-convert-calendar branch April 30, 2020 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants