search

opera-adt / COMPASS

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

update hdf5 metadata structure and content #75

Closed LiangJYu closed 1 year ago

LiangJYu commented 1 year ago

The PR adds:

LiangJYu commented 1 year ago

FYI - In the last couple commits, I left some questions as comments in this PR's code.

vbrancat commented 1 year ago

Thank you, @LiangJYu. it looks much better right now. A couple of minor fixes and it should be ready to go:

  1. /science/SENTINEL1/CSLC/corrections/zero_doppler_time has the shape of the slant_range vector not of the azimuth. In addition, the corrections group should have the slant range and zero doppler time spacing of the correction LUTs
  2. In /science/SENTINEL1/CSLC/grids/, we should remove spacing of the burst in radar coordinates and add those in geocoded coordinates (like x_spacing, y_spacing). I would also move the center_frequency and slant_range_bandwidth to the metadata section where we collect the info on the input burst
  3. before, we had a section of the metadata where we had a bunch of info related to the input burst named s1ab_burst_metadata. Can we have it back and put center_frequencyandslant_range_bandwidth` there?
  4. The corrections have contributions in range and azimuth. At the moment, they are allocated as 2-D arrays. Not sure if it is best to separate the slant range and azimuth LUTs in a separate group or allocated them as 3D matrices
  5. we choose the snake case convention but we call the processing info group as processingInformation. Let's make sure to have one convention

After that, we are good to go. Awesome work :)

LiangJYu commented 1 year ago
  1. In /science/SENTINEL1/CSLC/grids/, we should remove spacing of the burst in radar coordinates and add those in geocoded coordinates (like x_spacing, y_spacing). I would also move the center_frequency and slant_range_bandwidth to the metadata section where we collect the info on the input burst

Can you point to where I'm using radar coordinates. I don't think I'm using radar coordinates anywhere in the metadata. In /science/SENTINEL1/CSLC/grids/, the CSLC axis are in geo coordinates.

LiangJYu commented 1 year ago
  1. before, we had a section of the metadata where we had a bunch of info related to the input burst named s1ab_burst_metadata. Can we have it back and put center_frequencyandslant_range_bandwidth` there?

Restore other datasets to s1ab_burst_metadata too?

LiangJYu commented 1 year ago
  1. The corrections have contributions in range and azimuth. At the moment, they are allocated as 2-D arrays. Not sure if it is best to separate the slant range and azimuth LUTs in a separate group or allocated them as 3D matrices

Like the following?

* corrections
  - doppler
    - azimuth (aka bistatic_delay)
    - range (aka geometry_steering_doppler)
vbrancat commented 1 year ago

Answering to your questions in one comment.

  1. Correct, you are not using radar data in that group. My point is that having metadata related to range/Doppler products in a group where we have geocoded data is confusing. I propose to restore the metadata group s1ab_burst_metadata and insert all this info in this group
  2. Yes, let's restore s1ab_burst_metadata
  3. I like your solution
hfattahi commented 1 year ago

Thanks @LiangJYu for the PR. I think this is very exciting. Based on a product that @vbrancat pointed me to here are some observations:

The bounding polygon does not seem correct to me. It seems to be in pixel while it should be geo-coordinates

bounding_polygon b'POLYGON ((17968 546, 15156 808, 7580 1514, 5992 1662, 2752 1964, 1369 2093, 1170 2112, 1156 2120, 1152 2125, 1144 2141, 1142 2152, 1161 2238, 1189 2315, 1880 4093, 1915 4114, 2111 4096, 2423 4067, 17945 2617, 18476 2567, 18824 2534, 18896 2527, 18904 2522, 18907 2517, 18121 745, 17973 549, 17968 546))'

I wonder if we should use lower case for burst_ID to be burst_id and the mission_ID to be mission_id.

For product version please make sure that a field in the run config exists for "product_version". The SAS should read that field and place in the product_version metadata. Maybe that is already in place? (Note that SDS will be responsible for populating the product_version).

shall we use epsg_code instead of projection?

The data type is complex64 (two float 32). I thought we are going with float16. Is that a separate PR?

I suggest to change slc['science/SENTINEL1/CSLC/metadata/processing_information/algorithms/geocoding'] to slc['science/SENTINEL1/CSLC/metadata/processing_information/algorithms/geocode_interpolator'] or geocode_interpolation

This is great to keep track the input radar geometry here slc['science/SENTINEL1/CSLC/metadata/processing_information/s1_burst_metadata'], would you please make sure all information to reconstruct the radarGridParameters exists? I don't see zerod Doppler start and stop time. I also don't see the stop_range. We need either stop time and stop range or width length of the radar grid to reconstruct the radar grid parameters.

Please change azimuth_steer_rate to azimuth_steering_rate

I think it will be really nice to be able to reconstruct the burst SLC object with metadata in the CSLC product. Basically here we could keep track of all these parameters: https://github.com/opera-adt/s1-reader/blob/main/src/s1reader/s1_burst_slc.py#L209 . We actually had this functionality when we had json format.

LiangJYu commented 1 year ago

For product version please make sure that a field in the run config exists for "product_version". The SAS should read that field and place in the product_version metadata. Maybe that is already in place? (Note that SDS will be responsible for populating the product_version).

There's a place holder for product_version here.

shall we use epsg_code instead of projection?

epsg_code is an attribute of the projection dataset. Do you want it in its own dataset?

The data type is complex64 (two float 32). I thought we are going with float16. Is that a separate PR?

Would the ISCE3 GSLC complex<float16> PR need to precede the change to complex32 here?

hfattahi commented 1 year ago

For product version please make sure that a field in the run config exists for "product_version". The SAS should read that field and place in the product_version metadata. Maybe that is already in place? (Note that SDS will be responsible for populating the product_version).

There's a place holder for product_version here.

shall we use epsg_code instead of projection?

epsg_code is an attribute of the projection dataset. Do you want it in its own dataset?

The data type is complex64 (two float 32). I thought we are going with float16. Is that a separate PR?

Would the ISCE3 GSLC complex<float16> PR need to precede the change to complex32 here?

Thanks for clarification for product version. The projection is fine as is I think.

Oh I see that we still use the raster interface here. I guess we have to come back to complex 32 with a point release.