-
Notifications
You must be signed in to change notification settings - Fork 147
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
Add i.saocom and i.sar toolsets #949
base: grass8
Are you sure you want to change the base?
Conversation
Thank you for these tools! While it is not a hard requirement at this point, it would be really good if you provide also tests for these tools. The tests are extremely useful for long-term maintenance and will soon become a crucial part of the day-to-day updates of the grass-addons repository. The tests can be written with 1) unittest-based tests which provide similar workflow experience as when writing a tool or an example or 2) pytest tests which are used experimentally now and may become more prominent later if successful. |
Thanks for your contribution! One thing:
Should the original |
Gracias @sase1988 !! I can't help it to mention that this contribution is Santiago's final work for the GRASS GIS course where students were offered different options of final work to pass the course. Proud teacher and friend here! 🤩 |
Hello Markus! My proposal is to do so, in order to have all the SAR tools inside the same toolset. Of course, after the addon is approved 😄 |
Having "all the SAR tools inside the same toolset" sounds reasonable. While potentially complicating this PR, the clearest way might be to resolve this within one PR given that the name is the same. The issue with this in general is that moving a tool under a group breaks We don't give any promises on which addons install (although maybe we should), so formally it is okay, but still, we are trying hard to keep things working in general. With this in mind, perhaps leaving the original in place, but marking it or even replacing it with the new one (100% duplication) is on the table from my perspective. |
Cool stuff. I am very much looking forward to testing the new addons. |
Ciao @neteler and all. I'm glad to see more cool stuff related to SAR! It makes complete sense to me to have everything in one place, feel free to remove the addon that I created if it is a replication. Cheers |
Dear Vaclav, Thanks a lot for your feedback. I am now working on building a test script for i.saocom.import based on the unittest structure.
I would appreciate any guidance on this, I am quite new to this stuff of creating tests and I'm very interested in learning about them ;) Cheers |
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.
I mostly check format stuff and language. I still find it difficult to understand how the i.saocom tools work, perhaps a general explanation plus a flow diagram could be included in the i.socom.html
|
||
This module makes use of the <b>GCPS.csv</b> file generated by <em>i.saocom.import</em> and associated to a specific image. This GCP file is associated to the image basename specified in the option <b>basename</b>. | ||
|
||
The tool will write the output to a Geotiff external file in the directory specified in the <b>dbase</b> option. The output files will be projected to the CS of the target location indicated in the option <b>location</b>. If the specified location does not exits, GRASS will create it with the CS EPSG:4326. The output map name will we the same as the input map, with the suffix <em>_geo</em> added at the end. |
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 tool will write the output to a Geotiff external file in the directory specified in the <b>dbase</b> option. The output files will be projected to the CS of the target location indicated in the option <b>location</b>. If the specified location does not exits, GRASS will create it with the CS EPSG:4326. The output map name will we the same as the input map, with the suffix <em>_geo</em> added at the end. | |
The tool will write the output to a Geotiff external file in the directory specified by the <b>dbase</b> option. The output files will be projected to the CS of the target location indicated in the option <b>location</b>. If the specified location does not exist, GRASS will create it with the CS EPSG:4326. The output map name will be the same as the input map, with the suffix <em>_geo</em> added at the end. |
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.
Why do you need to specify a target location to get the geocoded data? The module will always be run from a certain location, hence that should be the target location, i.e., I'm running i.saocom.geocode from a UTM location, that's where I want the output to be, right? Hence no need for such a parameter.
|
||
|
||
if __name__ == "__main__": | ||
options, flags = grass.parser() |
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.
As a better practice, please move this into at the beginning of main function.
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.
What should I exactly move? the line options, flags = gscript.parser()
?
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, exactly
|
||
|
||
if __name__ == "__main__": | ||
options, flags = grass.parser() |
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.
Move this line into main()
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.
Same doubt as #949 (comment)
pid = str(os.getpid()) | ||
img_mean = "tmp%s_img_mean" % pid | ||
img_sqr = "tmp%s_img_sqr" % pid | ||
img_sqr_mean = "tmp%s_img_sqr_mean" % pid | ||
img_variance = "tmp%s_img_variance" % pid | ||
img_weights = "tmp%s_img_weights" % pid |
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.
Please see best practices for creating temporary maps:
https://github.com/wenzeslaus/foss4g-2022-developing-custom-grass-tools/blob/main/notebook_3_best_practices.ipynb (look for Temporary Maps section)
(This will eventually end up in official documentation...)
The idea is to create safe names, register them so that they get removed if something fails, and avoid accidental removal of maps created by different processes.
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 this comment. I have changed it trying to follow the recommended practices, but just in case I commented the original code lines.
Co-authored-by: Veronica Andreo <[email protected]>
Co-authored-by: Veronica Andreo <[email protected]>
Co-authored-by: Veronica Andreo <[email protected]>
Co-authored-by: Veronica Andreo <[email protected]>
Co-authored-by: Veronica Andreo <[email protected]>
Co-authored-by: Veronica Andreo <[email protected]>
Co-authored-by: Anna Petrasova <[email protected]>
Co-authored-by: Anna Petrasova <[email protected]>
Co-authored-by: Anna Petrasova <[email protected]>
Co-authored-by: Anna Petrasova <[email protected]>
Co-authored-by: Anna Petrasova <[email protected]>
Co-authored-by: Anna Petrasova <[email protected]>
Co-authored-by: Anna Petrasova <[email protected]>
Co-authored-by: Anna Petrasova <[email protected]>
Co-authored-by: Anna Petrasova <[email protected]>
Co-authored-by: Anna Petrasova <[email protected]>
Co-authored-by: Anna Petrasova <[email protected]>
Co-authored-by: Anna Petrasova <[email protected]>
Co-authored-by: Veronica Andreo <[email protected]>
Co-authored-by: Veronica Andreo <[email protected]>
Co-authored-by: Veronica Andreo <[email protected]>
Co-authored-by: Veronica Andreo <[email protected]>
Dear Anna, thank you very much for you feedback. Regarding the detail information, where or how (which format) should I give a more in-depth explanation? As suggested by @veroandreo I am drawing some graphical workflows to add to the .html files, but the detail about the external dependencies and how they are used in each step, and why I prefer not using more low level bindings such as GDAL, might be too much for the tool final user's document. |
Right, I think discussing it right here in the PR is appropriate. I still need to look at it in more detail, but it looks like you import GDAL package, but don't use it and instead call gdalwarp tool. Any reason why you use affine instead of rasterio.affine? In any case, the dependencies should be communicated in the manual and there should be lazy loading otherwise the addons won't be built. |
Dear Anna, |
I don't insist on replacing rasterio if that's a problem... I was trying to understand which dependencies are necessary and which ones were used e.g. just because of a better syntax. Generally fewer extra dependencies makes it simpler for the user. |
Looking at the diagrams, I was trying to understand the workflow and this is still unclear, I may be missing something: Also, as Vero noted earlier, the geocode tool should import it to the current location, not create a new one and import it there. Is there any reason it needs to be epsg 4326, it seems at that point any CRS should be acceptable, no? |
def read_bands_zip(data, pols): | ||
with ZipFile(data, "r") as zfile: | ||
file_list = [ | ||
img | ||
for img in zfile.namelist() | ||
if img.startswith("Data/") and not img.endswith(".xml") | ||
] | ||
bands = {} | ||
gcps = tuple() | ||
dataset_pols = [] | ||
for l in file_list: | ||
pol = l.split("-")[6] | ||
dataset_pols.append(pol) | ||
if pol in pols: | ||
bands[pol] = {} | ||
filename = f"/vsizip/{data}/{l}" | ||
with rasterio.open(filename) as banda: | ||
bands[pol]["array"] = banda.read() | ||
bands[pol]["metadata"] = banda.meta | ||
gcps = banda.get_gcps() | ||
return bands, gcps, dataset_pols |
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 is a lot of unnecessary code duplication. I would suggest using zipfile.is_zipfile
to detect if it's a zip file automatically (and remove the associated module option) and then merging these 2 functions.
The i.saocom.toolset is intended to read SAOCOM-1A L1A images. It currently comprises two basic modules:
The use of the modules can be tried with this sample SAOCOM dataset. It can be processed either as a zip file or as an uncompressed folder.
The i.sar toolset in inteded for general use on any SAR image. It currently comprises three modules. One already existed as a stand-alone tool (i.sar.speckle), and the other two are new.