-
Notifications
You must be signed in to change notification settings - Fork 12
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
Overwrite time array as average of time bounds if time bounds present #200
Conversation
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.
Thanks for putting this together. I agree; I do think _average_time_bounds
should go in utils.times
.
If I understand things correctly, I think there might be an issue in the logic for the case when time bounds is a 1D array (see my suggestion below). Other than that, there are just a few minor things.
aospy/data_loader.py
Outdated
bounds = ds[TIME_BOUNDS_STR] | ||
if bounds.ndim == 1: | ||
assert len(bounds) == len(ds[TIME_STR]) + 1 | ||
avg = 0.5*(bounds[:-1] + bounds[1:]) |
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.
Would this work? I think since bounds
is indexed by a 'time_bounds'
coordinate, xarray will try to align bounds[:-1]
with bounds[1:]
so in this case avg
would end up being the equivalent of bounds.isel(time_bounds=slice(1, -1))
(which is not the desired result).
I think something like the following is more what we're after?
avg = (0.5 * (bounds.shift(**{TIME_BOUNDS_STR: -1}) + bounds)).dropna(TIME_BOUNDS_STR)
E.g.
In [1]: import numpy as np
In [2]: import xarray as xr
In [3]: example = xr.DataArray(np.arange(5), coords=[np.arange(5)], dims=['time_bounds'])
In [4]: example
Out[4]:
<xarray.DataArray (time_bounds: 5)>
array([0, 1, 2, 3, 4])
Coordinates:
* time_bounds (time_bounds) int64 0 1 2 3 4
In [5]: 0.5 * (example[1:] + example[:-1])
Out[5]:
<xarray.DataArray (time_bounds: 3)>
array([ 1., 2., 3.])
Coordinates:
* time_bounds (time_bounds) int64 1 2 3
In [6]: example.isel(time_bounds=slice(1, -1))
Out[6]:
<xarray.DataArray (time_bounds: 3)>
array([1, 2, 3])
Coordinates:
* time_bounds (time_bounds) int64 1 2 3
In [7]: (0.5 * (example.shift(time_bounds=-1) + example)).dropna('time_bounds')
Out[7]:
<xarray.DataArray (time_bounds: 4)>
array([ 0.5, 1.5, 2.5, 3.5])
Coordinates:
* time_bounds (time_bounds) int64 0 1 2 3
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.
You're right; for some reason I was thinking about these as if they were just numpy arrays. Good catch!
aospy/data_loader.py
Outdated
assert len(bounds) == len(ds[TIME_STR]) + 1 | ||
avg = 0.5*(bounds[:-1] + bounds[1:]) | ||
elif bounds.ndim == 2: | ||
assert set([2, len(ds[TIME_STR])]) == set(bounds.shape) |
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 think
assert bounds.sizes == {TIME_STR: ds.sizes[TIME_STR], BOUNDS_STR: 2}
might be slightly more idiomatic xarray (because it takes into account the dimension label-size correspondence).
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.
Actually this should be:
assert dict(bounds.sizes) == {TIME_STR: ds.sizes[TIME_STR], BOUNDS_STR: 2}
because bounds.sizes
is an OrderedDict
(so we should cast it as an ordinary dict
first).
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.
@spencerkclark thanks for the review. All of your suggestions are good; will implement.
aospy/data_loader.py
Outdated
""" | ||
bounds = ds[TIME_BOUNDS_STR] | ||
if bounds.ndim == 1: | ||
assert len(bounds) == len(ds[TIME_STR]) + 1 |
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.
If this (or the other assert
statement below) raises an AssertionError
, should we provide a more informative error description?
I now realize that the 1-D time_bounds use case actually has never arisen for me. I was conflating it with the same behavior in AM2.1 but for lat and lon bounds:
So, unless somebody else has such a use case, I'm inclined to remove this logic, and only accept bounds arrays with shape |
I agree; I also have not encountered 1D time bounds, so this sounds like a good idea. |
Great. Also FWIW, CF conventions only permit the 2D format:
|
Also snuck in re-formatting of internal_names in utils.times.py
Given this, I'm wondering if we even need this check at all. It's hard to imagine an input dataset wherein the time bounds and time arrays are different lengths. So I'm inclined to just remove the check entirely. (Motivated by coveralls tipping me off that my test doesn't cover this mis-shaped case.) |
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.
@spencerahill thanks for taking the time to switch test_utils_times.py
to use pytest
! I have one optional suggestion that could simplify things a bit more, but it's totally up to you.
Regarding checking the shape of time_bounds
; I agree it is probably not necessary.
aospy/utils/times.py
Outdated
len(ds[TIME_STR])) | ||
raise ValueError(msg) | ||
|
||
start_bounds = bounds.isel(**{BOUNDS_STR: 0}) |
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.
The function you've written here is nice, because it can be applied to both dates and floats. That said, we only use this function on floats (i.e. before we decode times). Therefore optionally I think it would be possible to simplify the logic (and tests) slightly:
def average_time_bounds(ds):
bounds = ds[TIME_BOUNDS_STR]
new_times = bounds.mean(dim=BOUNDS_STR, keep_attrs=True).drop('time').rename(TIME_STR)
new_times[TIME_STR] = new_times
return new_times
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.
Note the slight edit above (the addition of .drop
)
aospy/data_loader.py
Outdated
ds = times.ensure_time_avg_has_cf_metadata(ds) | ||
ds[TIME_STR].values = times.average_time_bounds(ds).values |
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.
If you went with my suggestion for the simpler method below, I think you could simplify this to:
ds[TIME_STR] = times.average_time_bounds(ds)
aospy/test/test_utils_times.py
Outdated
@@ -530,5 +534,29 @@ def test_yearly_average_masked_data(): | |||
xr.testing.assert_allclose(actual, desired) | |||
|
|||
|
|||
if __name__ == '__main__': | |||
sys.exit(unittest.main()) | |||
def test_average_time_bounds(): |
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.
Regardless of whether or not you switch to a float-only method, could you add a test for the use case where floats are used rather than dates, since that is the use case in data_loader.py
?
Wow, you're absolutely right. Total mistake on my part. Lesson: it pays to stop and think before you start hacking away. Thanks for catching that. I implemented your suggested replacement. and it seems to work; will push when tests are good. |
I took the liberty of creating a pytest.fixture for the new test and rewriting one existing test, |
We're green. @spencerkclark I'll wait for your final approval, given how many careless mistakes I've made so far in this PR! 😝 |
Thanks, this looks good to me! |
Closes #185.
Maybe
_average_time_bounds
should go inutils.times
?Also still need:
- [ ] More informative message for the NotImplementedError