-
Notifications
You must be signed in to change notification settings - Fork 9
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
Average time #211
Merged
Merged
Average time #211
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
0157cab
add initial average_time functions
ffad882
add more tests for average time
1231a5f
merge master
645f4c1
update exception message
a07ce68
update history and docs
baf1b81
add time bounds and test for them
c16e787
move creation of time bounds to separate function
53860d0
change exception
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@ellesmith88 this is all looking good and the tests seem to cover everything we'd need.
There is one issue that we might need to consider philosophically (about what
xarray
is doing):In the test
test_average_over_years()
:In CF terms, we can think of the new time array as being a series of time periods rather than time points (i.e. time point 1 represents the entire year of 2005). In which case, the expected output array would typically be defined with a
time
axis andtime_bounds
bounds array.And typically the first time point would be:
However, that is not a definitive rule. I think the first thing to do is to test the values of the bounds. If the bounds come out correctly then we can probably just accept what
xarray
is doing.Please can you extend this test (and some of the others) to include a check for bounds values for time in a way that is logical.
Note that
xarray
has all these different frequencies:https://github.com/pydata/xarray/blob/main/xarray/coding/cftime_offsets.py#L605-L608
I'm not sure if the
YearEnd
andYearBegin
options might be relevant to this issue in some way.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.
@agstephens having a look at the output arrays, it seems that there are no time bounds.
For example, from the test
test_average_over_years()
, the output dataset is:The frequencies I'm using for calculating the time average are:
month: 1MS
year: 1AS
and I think the 'S' indicates using the start of the year or start of the month for resampling. I think this is probably what we want it to do, but I might be confused!
Other options include one such as AS-JUN, but I think this means the average is calculated over June - May rather than over Jan - Dec.
There are options in xarray resample (https://xarray.pydata.org/en/stable/generated/xarray.Dataset.resample.html) to change the label for the output times, but only to the left or right of the interval, so this isn't very helpful.
I can't see anything else about changing the label/ indicating the time period each time point covers, but will have another look to see what other options there might be
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.
Hi @ellesmith88, thanks for looking into this.
I wonder if they have looked into this within the CDS code. Please can you get in touch with James (ECMWF) to ask for his thoughts on this issue. I wonder if they have come across and tackled this problem - or whether they are just happy to accept what
xarray
produces.Thanks
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.
@ellesmith88 Just looking at your work so far on this. I would propose that we might want to re-use a
create_time_bounds(...)
function in other code (in future). So please can you move out that functionality into a separate function. I think it might get used outside ofclisops.core
andclisops.ops
so you could put them in a new module:clisops.utils.time_utils
.