Toniof 523 allow temporal resampling #542

TonioF · 2021-10-08T09:52:25Z

Solves #523 . xcube gen2 has now a parameter temporal_resampling to which any of the following parameters may be set for downsampling: 'count', first', 'last', 'min', 'max', 'sum', 'prod', 'mean', 'median', 'std', var', or 'percentile_<p>' and for upsampling: 'asfreq', 'ffill', 'bfill', 'pad', 'nearest', 'nearest-up', 'zero', 'slinear', 'quadratic', 'cubic', 'previous', 'next'.

Checklist:

Add unit tests and/or doctests in docstrings
Add docstrings and API docs for any new/modified user-facing classes and functions
New/modified features documented in docs/source/*
Changes documented in CHANGES.md
AppVeyor CI passes
Test coverage remains or increases (target 100%)

Remember to close associated issues after merge!

forman

Build fails, please fix first then request new review.

forman · 2021-10-13T10:02:40Z

CHANGES.md

+  'first', 'last', 'min', 'max', 'sum', 'prod', 'mean', 'median', 'std', 
+  'var', 'percentile_<p>']`, to sample up to a finer resolution, use any of 
+  `['asfreq', 'ffill', 'bfill', 'pad', 'nearest', 'nearest-up', 'zero', 
+  'slinear', 'quadratic', 'cubic', 'previous', 'next']`. (#523)


temporal_resampling should be an object that inludes parameters that control the resampling. Split up- and down-sampling explicitly, therefore include upsampling_method, downsampling_method as well as as optional upsampling_params, downsampling_params.

In principal, this sounds like a great idea that I would like to support, but it yields a larger problem: The temporal resolution of a dataset may be hard to determine. At times, it can be irregular and the decision whether downsampling or upsampling must be applied may not be unambiguous.
Keeping this in mind, I'll try to come up with a solution for this.

I have implemented that it is checked whether downsampling or upsampling should be applied.

test/core/gen2/test_config.py

test/core/gen2/test_request.py

xcube/core/gen2/config.py

xcube/core/gen2/local/resamplert.py

forman · 2021-10-13T10:09:27Z

xcube/core/gen2/local/resamplert.py

 class CubeResamplerT(CubeTransformer):

+    def __init__(self,
+                 cube_config: CubeConfig):


Please remove. I don't see a need to do this in the constructor. cube_config is passed to transform_cube().

If I remove this from the constructor, the parameter 'time_range' will most likely have been consumed by the CubeOpener and be unavailable to the CubeResamplerT. In consequence, the resampler would have no information about the requested start and ending time.
I see the following alternatives:
(a) ignore the time range in the resampler
(b) introduce a new parameter 'time_resample_time_range' so it would not be consumed earlier
(c) adapt the generator framework so that it does not remove parameters from a config

(a) If this is possible, then why do we have this discussion? Let's go for it.
(b) That doesn't make sense, because we have a parameter temporal_resampling to which we can time_range which could be by default the request's time_range.
(c) It MUST be possible to consume parameters, otherwise we don't know what the generator has to do. This is the idea of the current design.

There is also the simple solution to

(d) just pass the request's time range into the ctor.

I have now implemented this solution: The CubeConfig is not passed to the constructor anymore.
If transform_cube is called with a cubeconfig including time_range, it is used. Otherwise the time range is derived from the cube.

forman · 2021-10-13T10:11:23Z

xcube/core/resampling/temporal.py

+        return 29
+    if year % 100 == 0:
+        return 28
+    return 28


Can you pls explain why all this code is needed? Why can't we use pandas/xarray here?

pandas does not offer timedeltas in monthly, yearly or quarterly resolution, hence it is not possible to determine the bounds or adjust the value of a time step. However, this is necessary as pandas / xarray set the time value to the very start or end of the requested time period and thereby create confusion about the actual extent of the period.

pandas does not offer timedeltas in monthly, yearly or quarterly resolution

They changed this recently for very good reasons. Instead of "1M" you should say "4W" which is precise. Calendar month averaging can still be done. I don't see why we need all this code here.

See here http://xarray.pydata.org/en/stable/examples/monthly-means.html

See here http://xarray.pydata.org/en/stable/examples/monthly-means.html

DatasetResample and DataArrayResample both return GroupBy objects, so basically we are already using this. The problem is not the grouping itself but the determination of the exact times of the center of the sampling period and the bounds.
I have changed the code so that these are now determined using xarray's groupby.

# Conflicts: # CHANGES.md

codecov-commenter · 2021-10-15T17:27:17Z

Codecov Report

Merging #542 (b45051a) into master (f333bce) will decrease coverage by 0.11%.
The diff coverage is 85.38%.

@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
- Coverage   91.54%   91.43%   -0.12%     
==========================================
  Files         292      293       +1     
  Lines       26537    26990     +453     
==========================================
+ Hits        24294    24679     +385     
- Misses       2243     2311      +68

Impacted Files	Coverage Δ
xcube/core/gen2/local/resamplert.py	`64.84% <63.29%> (-35.16%)`	⬇️
xcube/core/resampling/temporal.py	`93.41% <90.43%> (-4.93%)`	⬇️
test/core/gen2/local/test_resamplert.py	`100.00% <100.00%> (ø)`
test/core/gen2/test_config.py	`100.00% <100.00%> (ø)`
test/core/gen2/test_request.py	`100.00% <100.00%> (ø)`
test/core/resampling/test_temporal.py	`100.00% <100.00%> (ø)`
test/util/test_jsonschema.py	`100.00% <100.00%> (ø)`
xcube/cli/resample.py	`90.80% <100.00%> (ø)`
xcube/core/gen2/config.py	`93.84% <100.00%> (+0.74%)`	⬆️
xcube/cli/io.py	`91.05% <0.00%> (-0.53%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f333bce...b45051a. Read the comment docs.

… arguments

forman · 2021-10-22T12:31:18Z

CHANGES.md

+  To sample down to a broader temporal resolution, use any of `['count', 
+  'first', 'last', 'min', 'max', 'sum', 'prod', 'mean', 'median', 'std', 
+  'var', 'percentile']`, to sample up to a finer resolution, use any of 
+  `['asfreq', 'ffill', 'bfill', 'pad', 'nearest', 'interpolate']`. (#523)


Not up to date with current impl.

I have updated this, but it has become quite long. The best solution would be to explain this parameter elsewhere and link there.

forman · 2021-10-22T12:32:54Z

test/core/gen2/local/test_resamplert.py

+        cube_config = CubeConfig(time_range=('2010-08-01', '2010-11-30'),
+                                 time_period='2M',
+                                 temporal_resampling=dict(
+                                     downsampling=('min', {}))


I'd like to be more pythonic here and allow for the no-args case also

Suggested change

downsampling=('min', {}))

downsampling='min'

forman · 2021-10-22T12:40:25Z

xcube/core/gen2/local/resamplert.py

 class CubeResamplerT(CubeTransformer):

+    def __init__(self,
+                 cube_config: CubeConfig):


(a) If this is possible, then why do we have this discussion? Let's go for it.
(b) That doesn't make sense, because we have a parameter temporal_resampling to which we can time_range which could be by default the request's time_range.
(c) It MUST be possible to consume parameters, otherwise we don't know what the generator has to do. This is the idea of the current design.

forman · 2021-10-22T12:41:27Z

xcube/core/gen2/local/resamplert.py

 class CubeResamplerT(CubeTransformer):

+    def __init__(self,
+                 cube_config: CubeConfig):


There is also the simple solution to

(d) just pass the request's time range into the ctor.

xcube/core/gen2/config.py

forman · 2021-10-22T12:51:43Z

xcube/core/resampling/temporal.py

+        return 29
+    if year % 100 == 0:
+        return 28
+    return 28


pandas does not offer timedeltas in monthly, yearly or quarterly resolution

They changed this recently for very good reasons. Instead of "1M" you should say "4W" which is precise. Calendar month averaging can still be done. I don't see why we need all this code here.

# Conflicts: # CHANGES.md

…rrayresample

TonioF added 12 commits September 17, 2021 15:16

work on integrating temporal resampling

ce44981

raise error when temporal resampling is requested

666633f

fix

367f0c2

integrated temporal resampling

b118f90

edited list of supported resampling methods

ef6a2a3

introduced temporal resampling

99f9551

improved temporal subsetting

6e9ca51

improved setting of bounds

6943c2a

adjust upsampled bounds with numpy timedelta correctly

3649eb3

Merge branch 'master' into toniof-523-allow-temporal-resampling

8d8d0f2

edited downsampling methods

b441a58

edited changelog

00eb10f

TonioF added the enhancement New feature or request label Oct 8, 2021

TonioF requested a review from forman October 8, 2021 09:52

forman requested changes Oct 11, 2021

View reviewed changes

TonioF added 3 commits October 11, 2021 17:46

Merge branch 'master' into toniof-523-allow-temporal-resampling

2dd3531

resample to center of month

6b76be5

avoid use of outdated method

89e26bd

forman self-requested a review October 13, 2021 09:58

forman requested changes Oct 13, 2021

View reviewed changes

TonioF added 4 commits October 14, 2021 09:30

Merge branch 'master' into toniof-523-allow-temporal-resampling

8b9a1aa

# Conflicts: # CHANGES.md

pep 8

3161ab2

work on better considering up- and downsampling

4177a4d

remodeled temporal resampling method parameter

19907c2

TonioF added 3 commits October 18, 2021 10:23

pass interpolation kind and percentile threshold as additional method…

9dca667

… arguments

added error handling

bffe1b3

edited changelog

a2d629b

TonioF requested a review from forman October 18, 2021 09:00

avoid use of deprectated methods

efbe060

forman requested changes Oct 22, 2021

View reviewed changes

TonioF added 7 commits January 10, 2022 17:03

Merge branch 'master' into toniof-523-allow-temporal-resampling

5f2a6e5

# Conflicts: # CHANGES.md

added tests

95a3394

allow passing in method name without parameters

e12b348

added empty line

e01c893

do not pass cube config in constructor

c098f6f

temporal resampling is done purely based on datasetresample and dataa…

15e6cf0

…rrayresample

updated changelog

a551b3c

TonioF requested a review from forman January 14, 2022 16:29

TonioF added 2 commits January 14, 2022 17:39

test fix

2e559b0

test fix

b45051a

TonioF self-assigned this Feb 22, 2022

TonioF linked an issue Feb 22, 2022 that may be closed by this pull request

Allow for temporal resampling in cube generator #523

Open

Toniof 523 allow temporal resampling #542

Are you sure you want to change the base?

Toniof 523 allow temporal resampling #542

Uh oh!

Conversation

TonioF commented Oct 8, 2021

Uh oh!

forman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TonioF Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TonioF Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TonioF Oct 14, 2021 •

edited

Loading

TonioF Jan 14, 2022 •

edited

Loading

codecov-commenter commented Oct 15, 2021 •

edited

Loading