Read average azimuth pixel spacing from S1 annotation files

[gshiroma]

This PR updates the S1-reader to parse the average azimuth pixel spacing from S1 annotation files and storing it as the Sentinel1BurstSlc class attribute average_azimuth_pixel_spacing. This field is required by CEOS ARD specifications for Normalized Radar Backscatter (NRB) products (e.g., the OPERA RTC-S1 product).

The PR also remove repeated code that reads the field imageAnnotation/imageInformation/numberOfSamples from S1 annotation files.

[seongsujeong]

It looks like the major point of the change is to provide the azimuth spacing in meters instead of seconds. I agree that the spacing in meters would be more helpful for the users, but I would like to hear from @hfattahi about this change.

The most recent CEOS ARD SAR spec does mention to include azimuth spacing in 1.6.7, but does not specify what unit to use. Based on several factors, I would like to minimize the changes on the source code unless it is absolutely necessary.

[gshiroma]

It looks like the major point of the change is to provide the azimuth spacing in meters instead of seconds. I agree that the spacing in meters would be more helpful for the users, but I would like to hear from @hfattahi about this change.

The most recent CEOS ARD SAR spec does mention to include azimuth spacing in 1.6.7, but does not specify what unit to use. Based on several factors, I would like to minimize the changes on the source code unless it is absolutely necessary.

Thank you, @seongsujeong ! I agree. Let's wait for @hfattahi 's opinion on this.

[seongsujeong]

LGTM. Thanks for the patch.

Aside from the code, my suggestion about the release is that we might need to do something similar to what we're doing on ISCE3 for CSLC-S1 patch release: it would be better to branch out from 0.2.2, apply the patch, and make use of that version for the patch release for RTC-S1. The version for s1reader would be something like 0.2.2.1. That is not a typical nomenclature for the versioning for OPERA, but it's not my first time to see such format in other projects.

I think cherrypicking will to the trick, but I'll try out few options after getting this PR merged to the main branch.

[hfattahi]

LGTM. Thanks for the patch.

Aside from the code, my suggestion about the release is that we might need to do something similar to what we're doing on ISCE3 for CSLC-S1 patch release: it would be better to branch out from 0.2.2, apply the patch, and make use of that version for the patch release for RTC-S1. The version for s1reader would be something like 0.2.2.1. That is not a typical nomenclature for the versioning for OPERA, but it's not my first time to see such format in other projects.

I think cherrypicking will to the trick, but I'll try out few options after getting this PR merged to the main branch.

@seongsujeong I understand your concern but in this case there has not been any changes to s1reader since the previous opera delivery. So I think we are not imposing unwanted changes here for a patch release. For example see here where we use s1reader 0.2.3. So I think we should be fine to bump the version up to 0.2.4. What do you think?

[hfattahi]

LGTM. Thanks @gshiroma

[gshiroma]

LGTM. Thanks @gshiroma

Thank you, @hfattahi , for reviewing this PR! I talked to @seongsujeong offline and his biggest concern is the "polygon issue" reported by @scottstanie here. All the datasets reported by @scottstanie are from 2015, so that shouldn't affect forward production. Also, from a first verification, it seems that the handling those bad datasets will be the same, i.e, raising and error and stop the processing. But I'd like to run more tests today to make sure everything is OK.

[seongsujeong]

@hfattahi @gshiroma Thanks for the feedback. Here is a bit more context about the versioning

• After the discussion, we decided to let the SAS fail when we cannot parse the polygon from the S1 IW annotation files
• But the way to make the software fail is different between RTC-S1 and CSLC-S1
  • RTC-S1 is using s1reader 0.2.2. The SAS failure takes place in s1reader side
  • CSLC-S1 is using s1reader 0.2.3. The SAS failure takes place in COMPASS side. This version fixes the missing data issue for azimuth FM rate data, and the polygon (it will return empty geometry)
  • The reasoning behind returning the empty polygon was because we wanted to read the S1 IW SLC bursts regardless of the missing metadata, and do something other than the OPERA L2 workflow.

Below are the solutions I can think about:

1. Merge this PR, bump the version to 0.2.4 and let RTC-S1 use that
   1.1. We might need to modify RTC-S1 to let the SAS fail

2. Post another PR to let s1reader
   2.1. That will make the reader fail to load some old SAFE files example here.

3. Use derivative version number like I've suggested above
   3.1. Probably the simplest solution
   3.2. Might need to branch out from the tag v0.2.2, cherrypick the commits in this PR to that branch

Please let us know what you think

[hfattahi]

@hfattahi @gshiroma Thanks for the feedback. Here is a bit more context about the versioning

• After the discussion, we decided to let the SAS fail when we cannot parse the polygon from the S1 IW annotation files

• But the way to make the software fail is different between RTC-S1 and CSLC-S1

  • RTC-S1 is using s1reader 0.2.2. The SAS failure takes place in s1reader side
  • CSLC-S1 is using s1reader 0.2.3. The SAS failure takes place in COMPASS side. This version fixes the missing data issue for azimuth FM rate data, and the polygon (it will return empty geometry)
  • The reasoning behind returning the empty polygon was because we wanted to read the S1 IW SLC bursts regardless of the missing metadata, and do something other than the OPERA L2 workflow.

Below are the solutions I can think about:

1. Merge this PR, bump the version to 0.2.4 and let RTC-S1 use that
   1.1. We might need to modify RTC-S1 to let the SAS fail
2. Post another PR to let s1reader
   2.1. That will make the reader fail to load some old SAFE files example here.
3. Use derivative version number like I've suggested above
   3.1. Probably the simplest solution
   3.2. Might need to branch out from the tag v0.2.2, cherrypick the commits in this PR to that branch

Please let us know what you think

Thanks @seongsujeong for pointing out those details. I'm ok with your Option-1.