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

Add i.saocom and i.sar toolsets #949

Open
wants to merge 38 commits into
base: grass8
Choose a base branch
from
Open

Conversation

sase1988
Copy link

@sase1988 sase1988 commented Oct 10, 2023

The i.saocom.toolset is intended to read SAOCOM-1A L1A images. It currently comprises two basic modules:

  • i.saocom.import to import SAOCOM SLC data as real and imaginary bands for the specified polarimetric channels
  • i.saocom.geocode to project these images or derived products from radar to projected coordinates

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.

  • i.sar.speckle: This module already exists as an individual tool in GRASS addons, but other filtering options have been added.
  • i.sar.amplitude: Tool intended to calculate an amplitude image from two SAR real and imaginary bands
  • i.sar.pauliRGB: Tool intended to calculate the RGB bands used as input for the Pauli Composition

@wenzeslaus wenzeslaus added the new addon PR contains a new addon or issue proposes one label Oct 11, 2023
@wenzeslaus
Copy link
Member

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.

@neteler
Copy link
Member

neteler commented Oct 11, 2023

Thanks for your contribution!

One thing:

i.sar.speckle: This module already exists as an individual tool in GRASS addons, but other filtering options have been added.

Should the original src/imagery/i.sar.speckle/ be removed then? Otherwise we get (partially) duplicate code. What do you think @madi ?

@veroandreo
Copy link
Contributor

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! 🤩

@sase1988
Copy link
Author

sase1988 commented Oct 11, 2023

Thanks for your contribution!

One thing:

i.sar.speckle: This module already exists as an individual tool in GRASS addons, but other filtering options have been added.

Should the original src/imagery/i.sar.speckle/ be removed then? Otherwise we get (partially) duplicate code. What do you think @madi ?

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 😄

@wenzeslaus
Copy link
Member

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 g.extension the.tool.name.

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.

@ninsbl
Copy link
Member

ninsbl commented Oct 12, 2023

Cool stuff. I am very much looking forward to testing the new addons.

@madi
Copy link
Contributor

madi commented Oct 12, 2023

Thanks for your contribution!

One thing:

i.sar.speckle: This module already exists as an individual tool in GRASS addons, but other filtering options have been added.

Should the original src/imagery/i.sar.speckle/ be removed then? Otherwise we get (partially) duplicate code. What do you think @madi ?

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

@neteler neteler changed the title Add i.saocom toolset Add i.saocom and i.sar toolsets Oct 12, 2023
@sase1988
Copy link
Author

sase1988 commented Oct 21, 2023

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.

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've seen that there are many assert() methods that could be very useful. The script file I attach (I changed the extension to .txt) runs the import of a sample SAOCOM dataset and compares it to a reference raster map (assertRastersEqual). It also checks if the imported raster exists (assertRasterExists). Does it make sense using assertRastersEqual in this context?
  • The script can be run under certain conditions: That GRASS has been initiated in a mapset where the reference raster exists. Does this make sense? I've seen the same logic in other codes like test_r_centroids. In case it does, how or where should I indicate this "base conditions", how should sample data be available if the file are large in terms of disk space?

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

test_i_saocom_import.txt

Copy link
Contributor

@veroandreo veroandreo left a 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor

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.

src/imagery/i.sar/i.sar.amplitude/i.sar.amplitude.html Outdated Show resolved Hide resolved
src/imagery/i.sar/i.sar.amplitude/i.sar.amplitude.html Outdated Show resolved Hide resolved
src/imagery/i.sar/i.sar.amplitude/i.sar.amplitude.html Outdated Show resolved Hide resolved
src/imagery/i.sar/i.sar.pauliRGB/i.sar.pauliRGB.html Outdated Show resolved Hide resolved
src/imagery/i.sar/i.sar.pauliRGB/i.sar.pauliRGB.html Outdated Show resolved Hide resolved


if __name__ == "__main__":
options, flags = grass.parser()
Copy link
Contributor

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.

Copy link
Author

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()?

Copy link
Contributor

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()
Copy link
Contributor

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()

Copy link
Author

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)

Comment on lines 72 to 77
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
Copy link
Contributor

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.

Copy link
Author

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.

sase1988 and others added 16 commits November 10, 2023 09:23
@sase1988
Copy link
Author

@sase1988 Looking at the saocom tools, could you explain what are they actually doing, I am not familiar with SAR data much. My specific issue is that you are using a lot external dependencies. Some of them seem unused. I would like to avoid as many as possible. I would prefer using osgeo package (python gdal bindings) rather than rasterio since osgeo is already used in other GRASS tools and I would think it would have all the functionality you need (maybe not?). But in general, I feel like the entire workflow of exporting and importing and creating new location could be improved from the point of integration with GRASS, but it would be helpful to have little more explanation.

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.

@petrasovaa
Copy link
Contributor

@sase1988 Looking at the saocom tools, could you explain what are they actually doing, I am not familiar with SAR data much. My specific issue is that you are using a lot external dependencies. Some of them seem unused. I would like to avoid as many as possible. I would prefer using osgeo package (python gdal bindings) rather than rasterio since osgeo is already used in other GRASS tools and I would think it would have all the functionality you need (maybe not?). But in general, I feel like the entire workflow of exporting and importing and creating new location could be improved from the point of integration with GRASS, but it would be helpful to have little more explanation.

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.

@sase1988
Copy link
Author

sase1988 commented Nov 13, 2023

@sase1988 Looking at the saocom tools, could you explain what are they actually doing, I am not familiar with SAR data much. My specific issue is that you are using a lot external dependencies. Some of them seem unused. I would like to avoid as many as possible. I would prefer using osgeo package (python gdal bindings) rather than rasterio since osgeo is already used in other GRASS tools and I would think it would have all the functionality you need (maybe not?). But in general, I feel like the entire workflow of exporting and importing and creating new location could be improved from the point of integration with GRASS, but it would be helpful to have little more explanation.

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 am working on replacing the use of rasterio with GDAL. It makes the code a little bit longer, but in the end it will use GDAL only. The main complication is to find a GDAL simple method to do the same as rasterio.transform.from_gcps(). I have also eliminated many libraries that indeed were unused.

@petrasovaa
Copy link
Contributor

Dear Anna, I am working on replacing the use of rasterio with GDAL. It makes the code a little bit longer, but in the end it will use GDAL only. The main complication is to find a GDAL simple method to do the same as rasterio.transform.from_gcps(). I have also eliminated many libraries that indeed were unused.

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.

@petrasovaa
Copy link
Contributor

Looking at the diagrams, I was trying to understand the workflow and this is still unclear, I may be missing something:
Why are these 2 modules and not one, just the geocode? What is the purpose of importing it into a temporary XY location and then exporting tif from there, doing the geocoding via external tools and then importing it again, it seems the temporary XY location step is not doing anything?

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?

Comment on lines +121 to +141
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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new addon PR contains a new addon or issue proposes one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants