t.rast.aggregate: Adjustments to temporally weighted aggregation #1799
Conversation
|
This addition sounds really cool :) Would you mind providing a reproducible example to test it? |
There was a problem hiding this comment.
Generally, this looks good!
I noticed couple of issues with the code. Some seem to related to some recent updates. Maybe just a better sync to the master branch is needed.
An example in the HTML manual page would be nice. A test would be great too. Most of temporal framework has a good test coverage, so it would be good to keep that. Creating a test data might be simpler than creating a meaningful example (although that would certainly be great too).
python/grass/temporal/aggregation.py
Outdated
| try: | ||
| for map_layer in granule.overlaps: | ||
| set_list.add(map_layer) | ||
| except: | ||
| pass |
There was a problem hiding this comment.
This seems to be similar to the code above, but it is written in a completely different style. Any reason not to follow the above style? Additionally, the try-except is unclear as it is written now since it is a bare except. See Flake8: ./temporal/aggregation.py:326:13: E722 do not use bare 'except' (also in CI).
python/grass/temporal/aggregation.py
Outdated
| string = name | ||
| weight = aggregation_weights[i] | ||
| file.write("%s|%f\n" % (string, weight)) | ||
| file.close() |
There was a problem hiding this comment.
The file.close() call is duplicated in the branches and does not need to be. Since you are rewriting most of the file code here, do you want to just use with statement avoiding the need for explicit close() call?
python/grass/temporal/aggregation.py
Outdated
| if None in t_granule_contained or None in t_granule: | ||
| aggregation_weights.append(0) | ||
| else: | ||
| overlap_abs = min(t_granule[1], t_granule_contained[1]) - max( | ||
| t_granule[0], t_granule_contained[0] | ||
| ) |
There was a problem hiding this comment.
(In-code) comments for these lines would be appreciated. Easier to review, easier to maintain when the intention is documented.
|
To be fixed: |
Review VP
|
Hi @wenzeslaus, we finally implemented the requested changes (some code restructuring & commenting, an example in the .html, added a test). Could you re-review? Thanks! |
| t_rast_list = SimpleModule("t.rast.list", input="B", flags="u").run() | ||
| rasters = [ | ||
| item | ||
| for item in t_rast_list.outputs.stdout.split(os.linesep) |
There was a problem hiding this comment.
There is splitlines for these cases (which actually behaves as expected unlike os.linesep here). (For future reference, see #2258, hopefully more straightforward.)
temporal/t.rast.aggregate/testsuite/test_aggregation_absolute.py
Outdated
Show resolved
Hide resolved
|
Binder link for anyone who wants to test this. |
After the merge with main, the link does not work. Use this one instead: https://mybinder.org/v2/gh/fkroeber/grass/t.rast.aggregate?urlpath=lab%2Ftree%2Fdoc%2Fnotebooks%2Fbasic_example.ipynb I tested by adapting the example in the html: the aggregation seems to work fine, but it's difficult to say if the weighting also works... Maybe a more intuitive example with r.mapcalc and maps having a single integer value? |
Anyone willing to propose an example here? |
| MAPS="map_1 map_2 map_3 map_4 map_5 map_6 map_7 map_8 map_9 map_10" | ||
| for map in ${MAPS} ; do | ||
| r.surf.random output=${map} min=278 max=298 | ||
| echo ${map} >> map_list.txt |
There was a problem hiding this comment.
| MAPS="map_1 map_2 map_3 map_4 map_5 map_6 map_7 map_8 map_9 map_10" | |
| for map in ${MAPS} ; do | |
| r.surf.random output=${map} min=278 max=298 | |
| echo ${map} >> map_list.txt | |
| for i in `seq 0 11` ; do | |
| r.mapcalc expression="map_${i} = ${i}" | |
| echo map_${i} >> map_list.txt |
There was a problem hiding this comment.
This example makes it much easier to understand what the weight really does as values in input maps are known and constant integer values
| basename=tendaily_temp_weighted method=average granularity="10 days" \ | ||
| sampling=related -w | ||
| </pre></div> | ||
|
|
There was a problem hiding this comment.
| Let's see the result: | |
| <div class="code"><pre> | |
| t.rast.list input=temperature_10daily_weighted columns=name,start_time,end_time,min,max | |
| name|start_time|end_time|min|max | |
| tendaily_temp_weighted_2021_05_01|2021-05-01 00:00:00|2021-05-11 00:00:00|0.3|0.3 | |
| tendaily_temp_weighted_2021_05_11|2021-05-11 00:00:00|2021-05-21 00:00:00|1.6|1.6 | |
| tendaily_temp_weighted_2021_05_21|2021-05-21 00:00:00|2021-05-31 00:00:00|3.1|3.1 | |
| tendaily_temp_weighted_2021_05_31|2021-05-31 00:00:00|2021-06-10 00:00:00|4.5|4.5 | |
| tendaily_temp_weighted_2021_06_10|2021-06-10 00:00:00|2021-06-20 00:00:00|5.9|5.9 | |
| tendaily_temp_weighted_2021_06_20|2021-06-20 00:00:00|2021-06-30 00:00:00|7.4|7.4 | |
| tendaily_temp_weighted_2021_06_30|2021-06-30 00:00:00|2021-07-10 00:00:00|8.7|8.7 | |
| tendaily_temp_weighted_2021_07_10|2021-07-10 00:00:00|2021-07-20 00:00:00|10.3|10.3 | |
| tendaily_temp_weighted_2021_07_20|2021-07-20 00:00:00|2021-07-30 00:00:00|11.0|11.0 | |
| <pre><div> | |
See my suggestions. What do you think? |
github-actions
bot
added
temporal
|
Solved conflicts |
echoix
changed the title
Until now, t.rast.aggregate provides diffrent sampling methods to select rasters for each time interval which subsequently are aggregated to the specified granularity using the specified method (in turn being handed over to r.series internally). Unfortunately, the present range of sampling methods only allow for complete in-/exclusion of single rasters during the aggregation procedure. This PR aims at enhancing t.rast.aggregate in a way that allows for weighted aggregations based on weighting factors representing the temporal overlap between the input rasters and the output rasters (see below). Therefore, a new sampling method called "relates" is introduced and a weighting flag (w) defined.