-
-
Notifications
You must be signed in to change notification settings - Fork 307
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
t.rast.univar: Add region_relation option for spatial filtering STDS by computational region #2793
Conversation
I went through the changes in the test file, looks good to me 👍 As per your questions:
|
Thanks, @veroandreo for looking into this and your feedback. Will add something to the documentation and turn the error into a warning then. |
@veroandreo Now I added some documentation. Please have a look, if that is an OK basis for adding functionality to other relevant modules!?! |
Is this meant for 8.3.0? Given that it is a first of a series of PRs, there might be some benefit in leaving this for 8.4.0. |
elif spatial_relation == "is_contained": | ||
spatial_where_template += " AND top <= {t}" " AND bottom >= {b}" | ||
elif spatial_relation == "contains": | ||
spatial_where_template += " AND top >= {t}" " AND bottom <= {b}" | ||
elif spatial_relation == "is_contained": | ||
spatial_where_template += " AND top <= {t}" " AND bottom >= {b}" | ||
elif spatial_relation == "contains": | ||
spatial_where_template += " AND top >= {t}" " AND bottom <= {b}" |
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.
This piece is a bug fix, so having it in a separate PR to go to 8.3.0 immediately would be nice, but it depends on how serious that is. Does not actually cause the code to fail?
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.
Yes, the code would fail, but the library function is not yet used... Could indeed be separated out, maybe together with corrections / improvements of the library function doc....
@@ -126,6 +127,12 @@ def print_gridded_dataset_univar_statistics( | |||
:param rast_region: If set True ignore the current region settings | |||
and use the raster map regions for univar statistical calculation. | |||
Only available for strds. | |||
:param dict spatial_relation: Spatial relation to the provided |
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.
It is str not dict. Same issue is the in the documentation of underlying functions.
err = "Space time %(sp)s dataset <%(i)s> is empty" | ||
warn = "Space time %(sp)s dataset <%(i)s> is empty" | ||
if spatial_relation: | ||
warn += " or no maps with the requested spatial_relation to the computational region exist" | ||
if where: | ||
err += " or where condition is wrong" | ||
gs.fatal( | ||
_(err) % {"sp": sp.get_new_map_instance(None).get_type(), "i": sp.get_id()} | ||
warn += " or where condition does not return any maps" | ||
gs.warning( | ||
_(warn) % {"sp": sp.get_new_map_instance(None).get_type(), "i": sp.get_id()} |
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 original code is wrong. The message cannot be constructed using +=
, but .format()
needs to be used (or maybe f-string in some cases). The _()
call needs to be around an actual string (which can be later combined with others).
Additionally, in major rewrites like this, try to replace things like %(sp)s
by .format()
(or f-strings if the base string is not translatable).
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.
Addressed in cf49c4e
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 .format()
call needs to be on the result of _()
. _()
needs to be around the actual string "..."
.
Additionally, a sentence cannot be constructed from multiple strings (regardless of what is used to construct it, +=
or multiple function calls). While this particular case may work for most languages, generally, you need to work around it, here, e.g., by stating something general and then creating a list of possible detailed explanations.
This should result in one verbose(...)
call which probably cleanest and fastest way of producing the message.
I agree with @veroandreo that the it is simply an empty output, not an error. I would add that this usually the desired behavior when batch processing and automating. There is couple of these cases in the code. For db.univar, the new JSON output gives "nulls" while the original output formats fails (PR code). Notably, I decided not to include any warning at all. Text won't help in automation, but you can always test for it in the parsed JSON if needed. I think it can be argued that warning is not appropriate, it is an expected output given the inputs, but a verbose message might be useful when you did not expect the output and need to investigate. |
As for the concept, I think the functionality is useful, but the option name is unclear. Spatial relation to what? The underlying function uses spatial relation but in combination with spatial extent. However, the new interface is always spatial relation to computational region extent. If your plan is to add spatial relation to other things besides the computational region extent, then the name makes sense. I'm not sure why |
Co-authored-by: Veronica Andreo <veroandreo@gmail.com>
Co-authored-by: Veronica Andreo <veroandreo@gmail.com>
Changed to verbose message... |
Renamed the module option to
Indeed, removed the rule.... |
I am happy to separate out bug-fixes for 8.3 and keep the rest for 8.4, esp if a first 8.3 RC is around the corner. However, if 8.3 is on a more long term schedule, the alternative would be to speed up adding functionality to other relevant modules. As of now, there is no 8.3 branch yet, so the PR would practically be on hold until the 8.3 branch is created... |
@neteler can comment on the schedule for 8.3, but in last few days, number of open PRs against main was reduced from >80 to <40 (by merging or moving the milestone), so we are getting reading for branching out. |
Seems all blockers are gone. Will write to grass-dev asap as the discussion doesn't fit here in a PR. |
err = "Space time %(sp)s dataset <%(i)s> is empty" | ||
warn = "Space time %(sp)s dataset <%(i)s> is empty" | ||
if spatial_relation: | ||
warn += " or no maps with the requested spatial_relation to the computational region exist" | ||
if where: | ||
err += " or where condition is wrong" | ||
gs.fatal( | ||
_(err) % {"sp": sp.get_new_map_instance(None).get_type(), "i": sp.get_id()} | ||
warn += " or where condition does not return any maps" | ||
gs.warning( | ||
_(warn) % {"sp": sp.get_new_map_instance(None).get_type(), "i": sp.get_id()} |
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 .format()
call needs to be on the result of _()
. _()
needs to be around the actual string "..."
.
Additionally, a sentence cannot be constructed from multiple strings (regardless of what is used to construct it, +=
or multiple function calls). While this particular case may work for most languages, generally, you need to work around it, here, e.g., by stating something general and then creating a list of possible detailed explanations.
This should result in one verbose(...)
call which probably cleanest and fastest way of producing the message.
Co-authored-by: Vaclav Petras <wenzeslaus@gmail.com>
Co-authored-by: Vaclav Petras <wenzeslaus@gmail.com>
Thanks for your code review effort, @wenzeslaus . Good catches. Should all be addressed now. |
@wenzeslaus Anything left to address here, or is the PR ready to merge? |
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.
Thank you for updating it!
OK to merge? I so, I will merge and take it as a template for adding spatial filtering to other temporal modules... |
…by computational region (OSGeo#2793) * add spatial extent filter * pass only spatial relation * add spatial extent filter * fix 3D where * add tests for spatial filter * add more tests for spatial filter * fix failing test * add spatial relation doc * lower message severity * spatial_relation intro * Update temporal/t.rast.univar/t.rast.univar.html Co-authored-by: Veronica Andreo <veroandreo@gmail.com> * Update temporal/temporalintro.html Co-authored-by: Veronica Andreo <veroandreo@gmail.com> * rewrite user message * fix tests for empty strds * rename spatial filter option * rename spatial filter option * update r-flag description * rename spatial filter option * rename spatial filter option * improve spatial filter docs * fix failing test * Update python/grass/temporal/abstract_space_time_dataset.py Co-authored-by: Vaclav Petras <wenzeslaus@gmail.com> * Update python/grass/temporal/univar_statistics.py Co-authored-by: Vaclav Petras <wenzeslaus@gmail.com> * address code review * address code review * address code review, some linting * add back percentiles * black --------- Co-authored-by: Veronica Andreo <veroandreo@gmail.com> Co-authored-by: Vaclav Petras <wenzeslaus@gmail.com>
…by computational region (OSGeo#2793) * add spatial extent filter * pass only spatial relation * add spatial extent filter * fix 3D where * add tests for spatial filter * add more tests for spatial filter * fix failing test * add spatial relation doc * lower message severity * spatial_relation intro * Update temporal/t.rast.univar/t.rast.univar.html Co-authored-by: Veronica Andreo <veroandreo@gmail.com> * Update temporal/temporalintro.html Co-authored-by: Veronica Andreo <veroandreo@gmail.com> * rewrite user message * fix tests for empty strds * rename spatial filter option * rename spatial filter option * update r-flag description * rename spatial filter option * rename spatial filter option * improve spatial filter docs * fix failing test * Update python/grass/temporal/abstract_space_time_dataset.py Co-authored-by: Vaclav Petras <wenzeslaus@gmail.com> * Update python/grass/temporal/univar_statistics.py Co-authored-by: Vaclav Petras <wenzeslaus@gmail.com> * address code review * address code review * address code review, some linting * add back percentiles * black --------- Co-authored-by: Veronica Andreo <veroandreo@gmail.com> Co-authored-by: Vaclav Petras <wenzeslaus@gmail.com>
…by computational region (OSGeo#2793) * add spatial extent filter * pass only spatial relation * add spatial extent filter * fix 3D where * add tests for spatial filter * add more tests for spatial filter * fix failing test * add spatial relation doc * lower message severity * spatial_relation intro * Update temporal/t.rast.univar/t.rast.univar.html Co-authored-by: Veronica Andreo <veroandreo@gmail.com> * Update temporal/temporalintro.html Co-authored-by: Veronica Andreo <veroandreo@gmail.com> * rewrite user message * fix tests for empty strds * rename spatial filter option * rename spatial filter option * update r-flag description * rename spatial filter option * rename spatial filter option * improve spatial filter docs * fix failing test * Update python/grass/temporal/abstract_space_time_dataset.py Co-authored-by: Vaclav Petras <wenzeslaus@gmail.com> * Update python/grass/temporal/univar_statistics.py Co-authored-by: Vaclav Petras <wenzeslaus@gmail.com> * address code review * address code review * address code review, some linting * add back percentiles * black --------- Co-authored-by: Veronica Andreo <veroandreo@gmail.com> Co-authored-by: Vaclav Petras <wenzeslaus@gmail.com>
This PR adds a
spatial_relation
filter tot.rast.univar
that makes the module compute univariate statistis only for maps with the given spatial relation to the computational region.It also fixes an issue in the temporal library.
This is intended to be a first of a series of PRs that add the spatial filter module to other relevant temporal modules. So, the PR is in a way a template and a careful review would be very much appreciated.
Tests are added for the new option.
Two questions in this regards:
where
clause or because the computational region is else where, should that really lead to a fatal error? Or is not a warning and no output more appropriate?