opera-adt / COMPASS

COregistered Multi-temPorAl Sar Slc
Apache License 2.0
38 stars 18 forks source link

Set az conversion #132

Closed seongsujeong closed 1 year ago

seongsujeong commented 1 year ago

This PR is a replacement of #105 which was closed accidentally.

Most of the comments / suggestions were applied in the previous PR.

Below is my comment regarding the suggestion to separate enu2rgaz into enu2rg and en2az: https://github.com/opera-adt/COMPASS/pull/105#discussion_r1157843023

vbrancat commented 1 year ago

@seongsujeong, while I was testing this PR, I noticed that the azimuth component of the Solid Earth Tides (SET) is not allocated inside the HDF5 CSLC product. However, it seems that the azimuth component of the correction is allocated inside the temporary product corrections in the scratch directory. Is this intentional? If not, I believe you should modify this function here https://github.com/opera-adt/COMPASS/blob/8aeecda5f43b15869a0d044b78df9d7b9864d5b3/src/compass/utils/h5_helpers.py#L583 and include the azimuth SET.

seongsujeong commented 1 year ago

@seongsujeong, while I was testing this PR, I noticed that the azimuth component of the Solid Earth Tides (SET) is not allocated inside the HDF5 CSLC product. However, it seems that the azimuth component of the correction is allocated inside the temporary product corrections in the scratch directory. Is this intentional? If not, I believe you should modify this function here

https://github.com/opera-adt/COMPASS/blob/8aeecda5f43b15869a0d044b78df9d7b9864d5b3/src/compass/utils/h5_helpers.py#L583 and include the azimuth SET.

@vbrancat Thanks for catching this. I've made the changes on the function to add the azimuth component of SET LUT be written into the output HDF5.

vbrancat commented 1 year ago

@scottstanie it would be great if you can have a look at this PR. Thanks

scottstanie commented 1 year ago
  1. Looks like changes to isce v0.12 means it's erroring because of azimuth_carrier being the wrong name for an argument:
    File "/u/aurora-r0/staniewi/repos/sweets/src/sweets/_geocode_slcs.py", line 59, in run_geocode
    s1_geocode_slc.run(cfg)
    File "/u/aurora-r0/staniewi/repos/COMPASS/src/compass/s1_geocode_slc.py", line 158, in run
    isce3.geocode.geocode_slc(geo_burst_raster, rdr_burst_raster,
    TypeError: geocode_slc() got an unexpected keyword argument 'azimuth_carrier'
  2. any chance of adding a unit test for some of the new/changed functionality?
scottstanie commented 1 year ago

after changing azimuth_carrier to az_carrier:

journal: radar input is invalid: is_list=False, is_array=False, n_items=0, shapes_consistent=False, dtypes_consistent=False, is_valid=False
Traceback (most recent call last):
  File "/u/aurora-r0/staniewi/miniconda3/envs/mapping/bin/s1_cslc.py", line 8, in <module>
    sys.exit(main())
  File "/u/aurora-r0/staniewi/repos/COMPASS/src/compass/s1_cslc.py", line 49, in main
    run(parser.run_config_path, parser.args.grid_type)
  File "/u/aurora-r0/staniewi/repos/COMPASS/src/compass/s1_cslc.py", line 43, in run
    s1_geocode_slc.run(cfg)
  File "/u/aurora-r0/staniewi/repos/COMPASS/src/compass/s1_geocode_slc.py", line 158, in run
    isce3.geocode.geocode_slc(geo_burst_raster, rdr_burst_raster,
  File "/home/staniewi/repos/isce3/install/packages/isce3/geocode/geocode_slc.py", line 162, in geocode_slc
    _io_valid(geo_io_checks, rdr_io_checks)
  File "/home/staniewi/repos/isce3/install/packages/isce3/geocode/geocode_slc.py", line 74, in _io_valid
    error_channel.log(err_str)
journal.ext.journal.ApplicationError: isce3.geocode.geocode_slc: application error

I also tried merging in the main branch to see if that helped, but the changes are failing:

journal: Validation fail ...
    runconfig.groups.product_path_group.product_version: '0.2' is not a str.
seongsujeong commented 1 year ago

@scottstanie In case you are wondering about the verification, the plots below is the comparison between the SET LUTs from this branch and this. Please note that I've found and fixed the issue regarding the azimuth angle conversion in the former one, and inverted the sign.

For the azimuth component, The former one gives the LUT in meters, whereas the latter one is in (Azimuth) time. So I multiplied the approximation of the ground speed of the satellite (~6.85km/s)

We see similar level of results in both range and azimuth direction calculated based on either radar geometry or coordinate conversion. Therefore I conclude that the output from this PR makes sense.

output_set_az

seongsujeong commented 1 year ago

@scottstanie I'm not sure if the errors you reported in the comment above is related to this PR. I know it can be a bit tricky to test this PR with the most up-to-date ISCE3, but if the change look good to you overall?