ACCESS-Cloud-Based-InSAR / dem-stitcher

Download and merge DEM tiles
Apache License 2.0
39 stars 15 forks source link

add nodata value for missing tile #53

Closed mgoacolou closed 1 year ago

mgoacolou commented 1 year ago
cmarshak commented 1 year ago

Hey Manual,

The PR is a good start.

Pros: doesn't affect default behavior.

Cons: Testing of new feature so that this doesn't impact downstream outputs. I force np.nan to be nodata value so I don't have to keep track of it - geoid removal assumes this.

The truth is, what is the advantage of doing this as opposed to running the API as is and then doing a slice like:

dem_array[np.isnan(dem_array)] = nodata_value

You could potentially include a line like that that at the end so nothing changes - but not sure if this is valuable. We can continue the conversation here.

mgoacolou commented 1 year ago

Hi,

this PR is a trick when using the DEM for RPC projection with GDAL in my case. GDAL doesn't show any data if the DEM is not filled with values:

Capture d’écran de 2023-01-11 08-08-09

the other side of the trick is that in this use case, even on sea, the missing tiles must filled constantly to geoid flag (0 or geoid values if remove)

Capture d’écran de 2023-01-11 08-11-21

So

May be a should had this trick in comment of the doc

cmarshak commented 1 year ago

I understand what you are trying to accomplish and where this library falls short.

The pictures you shared are the backscatter so it's not really helping me understand how dem-stitcher deals with the changes that you are proposing.

We will need add some integration tests based on the proposed changes. Again, I understand the goal (fill in missing data with 0s if possible), but want to make sure this is not going to lead to new errors.

For the time being - are you generating a DEM using your branch? Or with a small script after the fact?

Thanks for your interest to increase applicability of this library.

cmarshak commented 1 year ago

I added the tests that I had in mind - changes look really helpful. I will integrate these changes into the next release. Thanks, @mgoacolou, for your help!

mgoacolou commented 1 year ago

Hi, thanks for the merge, do you plan to make a release on PyPi ?