Skip to content
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

TGIS: fix semantic label #2215

Merged
merged 2 commits into from
Feb 16, 2022
Merged

TGIS: fix semantic label #2215

merged 2 commits into from
Feb 16, 2022

Conversation

metzm
Copy link
Contributor

@metzm metzm commented Feb 16, 2022

Do not treat a semantic label as shortcut or band identifier if it contains text. This PR is needed to support semantic labels such as red, green, blue, mask.

@metzm metzm added bug Something isn't working backport_needed temporal Related to temporal data processing labels Feb 16, 2022
@metzm metzm requested a review from neteler February 16, 2022 15:24
@metzm metzm added this to the 8.0.1 milestone Feb 16, 2022
 * do not modify the provided semantic label
@landam
Copy link
Member

landam commented Feb 16, 2022

@metzm Can you provide example of usage?

@metzm
Copy link
Contributor Author

metzm commented Feb 16, 2022

Test case, adapted from a test for t.rast.mapcalc:

g.region s=0 n=10 w=0 e=10 res=1 -p

# Generate data
r.mapcalc expr="prec_1 = rand(0, 550)" -s
r.mapcalc expr="prec_2 = rand(0, 450)" -s
r.mapcalc expr="prec_3 = rand(0, 320)" -s
r.mapcalc expr="prec_4 = rand(0, 510)" -s
r.mapcalc expr="prec_5 = rand(0, 300)" -s
r.mapcalc expr="prec_6 = rand(0, 650)" -s

t.create type=strds temporaltype=absolute output=precip_abs1 title="A test" descr="A test"

echo "prec_1|2001-01-01|2001-01-10|prec1" >testlist
echo "prec_2|2001-01-01|2001-01-10|prec2" >>testlist
echo "prec_3|2001-01-01|2001-01-10|prec3" >>testlist
echo "prec_4|2001-02-01|2001-02-10|prec1" >>testlist
echo "prec_5|2001-02-01|2001-02-10|prec2" >>testlist
echo "prec_6|2001-02-01|2001-02-10|prec3" >>testlist

t.register type=raster input=precip_abs1 file=testlist

t.info in=precip_abs1
t.rast.list in=precip_abs1 columns=name,semantic_label

t.rast.mapcalc inputs=precip_abs1.prec1,precip_abs1.prec2 expression="precip_abs1.prec1 / precip_abs1.prec2" output=precip_mapcalc basename=precip_mapcalc

@neteler neteler requested a review from marisn February 16, 2022 19:50
@neteler
Copy link
Member

neteler commented Feb 16, 2022

(I initially had an issue with the creation of testlist due to a double run of the test).

Tested successfully.

@metzm
Copy link
Contributor Author

metzm commented Feb 16, 2022

I would prefer to remove all modifications to the provided semantic label in _update_where_statement_by_semantic_label(), but this would be another PR.

@metzm metzm merged commit 843ffae into OSGeo:main Feb 16, 2022
@metzm metzm deleted the semantic_label branch February 16, 2022 21:11
@wenzeslaus
Copy link
Member

I would prefer to remove all modifications to the provided semantic label in _update_where_statement_by_semantic_label(), but this would be another PR.

Just from a quick look here, I agree. Perhaps the whole nested label syntax (and aliases?) can be dropped and replaced by simple look up.

metzm added a commit that referenced this pull request Feb 16, 2022
 * fix detection of a shortcut or band identifier
 * do not modify the provided semantic label if it contains `_`
@wenzeslaus
Copy link
Member

FYI, none of the t.rast.mapcalc tests actually runs automatically. Maybe all is needed is moving the files to a testsuite directory (there is none in the t.rast.mapcalc directory now). Bash scripts should just run at least in the Ubuntu CI and obviously locally on a right machine and the test above seems self-contained, so hopefully no trouble there either.

@metzm
Copy link
Contributor Author

metzm commented Feb 16, 2022

I prefer to just delete all modifications to the provided semantic label, requiring that the provided semantic label exists. The reason for these modifications to the provided semantic label is unknown: no code comments.

Alternatively, if the provided semantic label does not exist in the strds, try to modify it, but check if the modified label exists. That would be some overhead, querying the TGIS db is rather slow.

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
 * fix detection of a shortcut or band identifier
 * do not modify the provided semantic label if it contains `_`
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
 * fix detection of a shortcut or band identifier
 * do not modify the provided semantic label if it contains `_`
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
 * fix detection of a shortcut or band identifier
 * do not modify the provided semantic label if it contains `_`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working temporal Related to temporal data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants