Simplify raster band reference management

[marisn]

PR #63 introduced imagery raster band reference support.
This PR simplifies raster band management by moving band references into simple metadata items. Band references are not any more tied to a known sensor (e.g. "vi_NDVI" now is a valid band reference).
Band reference is now printed by r.info and can be managed with metadata editing module r.support.

[marisn]

On a second though current approach in band management is not flexible enough to untangle signature files from groups. I'll try to add some extra functionality to raster band references needed for further progress.

[wenzeslaus]

Long term goal – untangle signature files from groups and tie to band references.

Which brings back question which was not sufficiently answered before I think. What is the relation of band references and groups?

[wenzeslaus]

Since you are thinking about this and adding interface, any opinion on using the word band here, esp. in names of modules without the word reference? Given how the term band is usually used, i.band seems like referring to an object (the band reference) by adjective rather than a noun, i.e., going against the established meaning of band. I would just like to avoid another map-layer mess ("Check which layer of vector map you use when adding a new vector map layer to map in map display").

[marisn]

Since you are thinking about this and adding interface, any opinion on using the word band here, esp. in names of modules without the word reference? Given how the term band is usually used, i.band seems like referring to an object (the band reference) by adjective rather than a noun, i.e., going against the established meaning of band. I would just like to avoid another map-layer mess ("Check which layer of vector map you use when adding a new vector map layer to map in map display").

Woosh! (The sound your comment made flying high above my head)

[marisn]

Long term goal – untangle signature files from groups and tie to band references.

Which brings back question which was not sufficiently answered before I think. What is the relation of band references and groups?

None (sort of).
The way I see band references as an optional dimension name of any raster map. Such universal dimension identifier is required to make reusable signature files. Currently signatures are mapped to raster files by their order in group REF file. If map order in REF file changes, signature files become erroneous; There is no way to tell that the signature of group item 1 generated from map "a.red" is also valid for maps "b.red" and "c.red" (assuming a, b, c are form the same sensor). Thus my proposal would be to:

1. each raster map to have an optional band (dimension) name
2. map signatures to band (dimension) names (a.k.a. natural keys)

Thus a single signature file would be valid for any group as long as group members have the same band (dimension) names. Groups would stay as a mean of organizing a realization of set of bands (dimensions).

Downsides:

• existing signature files will become obsolete
• impossible to do classification of rasters without band names
  Impact of this issue could be minimised by:
  • automatic assigning of band name on import (if it is known)
  • automatic assigning of band name for various raster maps generated by GRASS tools (e.g. band "GRASS_slope_deg", "RI_NDVI")
  • improvement of metadata management tools (this is a good thing regardless of band management)

[neteler]

Something went wrong here, @marisn : Files changed 1,169?!
Forgot to update before push?

[nilason]

Something went wrong here, @marisn : Files changed 1,169?!
Forgot to update before push?

I think this is what happens after a rebase. Just normal behaviour as far as I know.

[wenzeslaus]

Something went wrong here, @marisn : Files changed 1,169?!
Forgot to update before push?

I think this is what happens after a rebase. Just normal behaviour as far as I know.

Unfortunately, something indeed went wrong as @neteler suggests. This is behavior when you rebase all other commits on top of yours. What you want when rebasing is rebasing your commits on top the other ones. git rebase upstream/master on your feature branch does that assuming your upstream/master is matching the OSGeo:master branch. The "right" rebase done without squash does not show anything special on GitHub except that you force pushed. With squash, it destroys commit history in the PR which may or may not be what you want (think e.g. review). Doing merge instead of the rebase is fine too. GitHub understands that and does shows only relevant changes, however it shows the merge commit. The merge commit disappears during Squash and merge.

I'm not sure what merging this would do, i.e., if Squash and merge would remove the extra commits. You can see on GitHub that @marisn is now additional author for each of these commits. However, I think it needs to be redone anyway because of the review.

I think the fix should be straightforward, identify your commits, create a new branch from updated master branch, cherry pick your commits, force push that branch to your remote branch bandrefs. When you check that all is fine, you can delete the local branch called bandrefs. (This is perfectly fine approach to completely redoing code of a PR without opening a new PR especially applicable when the original code nor the history does not have any value or the good parts will be transferred to the new branch and the original history is of no value for the review.)

[marisn]

I significantly simplified raster band references based on the fact that only ID part is used everywhere and filename part is used only to print band metadata and thus can be replaced with a search at runtime.

@landam what do you think about such approach? (This is still WIP)

Tests for some temporal modules are failing and I have hard time to understand why. For some strange reason during registration code takes a wrong branch here:

grass/python/grass/temporal/register.py

Line 176 in 802b3c8

if not sp.is_time_relative() and isinstance(time_object, datetime):

I also haven't understood what to do with i.band module. Band management now can be done via r.support (except printing). On one hand it could be kept as is (+ code upgrade) with additional explanations in documentation (+ link to r.support).

[marisn]

Tests for some temporal modules are failing and I have hard time to understand why. For some strange reason during registration code takes a wrong branch here:

grass/python/grass/temporal/register.py

Line 176 in 802b3c8

if not sp.is_time_relative() and isinstance(time_object, datetime):

Ah, I see that test temporal/t.rast.aggregate/testsuite/test_aggregation_relative.py also fails in master. I can fix it by following code, but, as I do not understand whole codebase, I can not judge if it is the right thing to do.

--- a/python/grass/temporal/register.py
+++ b/python/grass/temporal/register.py
@@ -173,7 +173,7 @@ def register_maps_in_space_time_dataset(
                 start_time_in_file = True
                 # Check if last column is an end time or a band reference
                 time_object = check_datetime_string(line_list[2])
-                if not sp.is_time_relative() and isinstance(time_object, datetime):
+                if sp.is_time_relative():
                     end_time_in_file = True
                     band_reference_in_file = False
                 else:

[marisn]

I have no idea why python parallel region test is failing as this PR is not touching anything in that area.

[nilason]

I have no idea why python parallel region test is failing as this PR is not touching anything in that area.

Rebase maybe helps.

[wenzeslaus]

I have no idea why python parallel region test is failing as this PR is not touching anything in that area.

Thanks for looking into the failing test! Not really related here. I'm still investigating this and meant to open an issue for it. You can see it happens time to time on master branch as well. At this point, I think it is caused by whatever file system is in GitHub Actions. The tests tries to write 50 rasters at once, so my though was some are not actually ready to be read from the disk yet when the test asks (for example, deleting a file on Windows takes some time to take effect). Suggestions welcome. Anyway, not related to this PR unless there are other tests failing.

Rebase maybe helps.

Not this time, but in these cases you can also try rerunning the jobs, may help this time and would lead to the correct conclusion about unrelated fluctuation in tests (which of course can work both ways).

[marisn]

Not this time, but in these cases you can also try rerunning the jobs, may help this time and would lead to the correct conclusion about unrelated fluctuation in tests (which of course can work both ways).

I can not reproduce the issue locally on Ubuntu 20.10. Unfortunately this could indicate on a Heisenbug in the code. It might be almost impossible to catch as the code could look file but e.g. something considered (and look like) to be atomic is not atomic at all and thus special timing or system settings would cause code to fail. Unless one of developers manages to reproduce it locally, it will remain with us for a long time (unless it is fixed by the PR #1627).

[wenzeslaus]

I don't want to distract from the PR topic too much, so just to conclude, so that someone can pick this up elsewhere:

Not this time, but in these cases you can also try rerunning the jobs...

I really meant rerunning the CI jobs. There is a button for it in the top right when you view job details.

I can not reproduce the issue locally on Ubuntu 20.10.

And I don't expect you would. My guess is it is file system related. I executed that test on my machine many times when working on #638.

...it will remain with us for a long time...

I think the big question will be if the specific conditions are something the code or the test should account for.

...unless it is fixed by the PR #1627.

I fixed the race condition related to that in #638, so I don't think there is another one related to region, at least not in the area of #1627.

[marisn]

Should I (finally) merge this PR?

[veroandreo]

Should I (finally) merge this PR?

all checks passed, +1 for merging so we can more easily test

[nilason]

Should I (finally) merge this PR?

No objections from me. +1