ASFHyP3 / hyp3-gamma

HyP3 plugin for generating SAR products with GAMMA
BSD 3-Clause "New" or "Revised" License
30 stars 7 forks source link

Update water mask URL #398

Closed jacquelynsmale closed 1 year ago

jacquelynsmale commented 2 years ago
cirrusasf commented 2 years ago

Using buffered method makes more water pixels into land pixels. In order to overcome this, we try to be more conservative to classify the shore line into land pixels. If this is our purpose for this update, then we should maybe classify the pixels which touch to the shoreline into water pixels.

asjohnston-asf commented 2 years ago

That's the intent behind setting allTouched=True in call to gdal.Rasterize(), so any edge-case pixels are classified as land rather than water. I like that change!

asjohnston-asf commented 2 years ago

@jacquelynsmale Implementation looks solid!

We should update the docstring for create_water_mask at https://github.com/ASFHyP3/hyp3-gamma/blob/develop/hyp3_gamma/water_mask.py#L22 to also mention Level 3, and to remove the reference to the 3km buffer.

We'll also want to remove the 3km reference from the InSAR README template at https://github.com/ASFHyP3/hyp3-gamma/blob/develop/hyp3_gamma/metadata/templates/insar/readme.md.txt.j2#L212 and the water mask metadata template at https://github.com/ASFHyP3/hyp3-gamma/blob/develop/hyp3_gamma/metadata/templates/insar/water_mask_tif.xml.j2#L9 . I haven't done an exhaustive search for other references to the buffer, yet.

Edit: looks like those are the only three hits when searching for "3 km" or "3km": https://github.com/ASFHyP3/hyp3-gamma/search?q=%223+km%22

jacquelynsmale commented 2 years ago

Responding to the water classification discussion: I wanted to include this image showing the classification of pixels from our test case. Transparent white represents pixels classified as land, brown is the input shapefile of land, black represents pixels classified as water, and the middle gray represents the pixels that were classified as water before the rasterize update but are classified as land with this change. The idea behind including any pixel that intersects with the land is to ensure we are excluding any land, especially since we know the shapefile isn't 100% accurate. Shapfileupdate

asjohnston-asf commented 1 year ago

I see a short-form story map URL in the README, but a long-form URL everywhere else. I'd consider using the short-form everywhere, assuming it's as long-lived as the longer URL? https://arcg.is/19aKHj https://storymaps.arcgis.com/stories/485916be1b1d46889aa436794b5633cb

hjkristenson commented 1 year ago

@asjohnston-asf I put the short URL in the readme because it has to be spelled out. I still don't quite trust the short URLs; for the webmaps they link to a specific version of the map, not necessarily the currently saved version. It looks like they're essentially interchangeable for StoryMaps, but I can't shake the feeling that it's more robust to use the full URL.

My approach was to use the short URL for the readme, where the full URL has to be displayed, and to use the long URL for the documentation where the actual URL doesn't need to be displayed.

asjohnston-asf commented 1 year ago

I've reviewed the secret analysis workflow and confirmed it's a false-positive for the story map URLs. Fine to merge.