Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Burst id and absolute orbit number #64

Merged
merged 5 commits into from
Aug 25, 2022

Conversation

seongsujeong
Copy link
Contributor

This PR updates the two things:

  • Burst ID: three digits for the track number. e.g. t051_, t123_
  • Absolute orbit number: For populating the RTC metadata, Sentinel1BurstSlc will keep the absolute orbit number as one of its attributes.

seongsujeong and others added 4 commits August 8, 2022 11:51

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Updating the main branch of this fork to update after `s1_annotation.py`
Copy link
Contributor

@gshiroma gshiroma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, as long as the changes are OK with @hfattahi , @LiangJYu , and @vbrancat

@LiangJYu
Copy link
Contributor

Before approving, may I ask what are the use cases for these changes?

@seongsujeong
Copy link
Contributor Author

Before approving, may I ask what are the use cases for these changes?

Yes. The new format of burst_id will make the lengths of the string to be the same.
RTC metadata parses some information from burst id. We can already extract what we need from the current format, but the new one will make it easier.
Regarding the absolute orbit number, we need this information to make the RTC metadata as similar as the NISAR-equivalent product (GCOV)

Copy link
Contributor

@LiangJYu LiangJYu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seongsujeong Thank you for the expanded explanation.

The unit tests need to be fixed to account for these changes by:

  1. updating burst_id format here.
  2. adding an assert for abs_orbit_number here

@seongsujeong
Copy link
Contributor Author

@LiangJYu Thanks for pointing me to the unit test. I've updated test_bursts.py in accordance with your suggestion.
Currently circieCI keeps on failing due to the error message below:

CondaHTTPError: HTTP 404 NOT FOUND for url <https://repo.anaconda.com/pkgs/main/linux-64/zlib-1.2.12-h7f8727e_1.conda>
Elapsed: 00:00.093111
CF-RAY: 740068947daf5b28-IAD

An HTTP error occurred when trying to retrieve this URL.
HTTP errors are often intermittent, and a simple retry will get you on your way.



Exited with code exit status 1
CircleCI received exit code 1

Hope this failure is not related to this PR.

Copy link
Contributor

@LiangJYu LiangJYu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LiangJYu
Copy link
Contributor

@LiangJYu Thanks for pointing me to the unit test. I've updated test_bursts.py in accordance with your suggestion. Currently circieCI keeps on failing due to the error message below:

CondaHTTPError: HTTP 404 NOT FOUND for url <https://repo.anaconda.com/pkgs/main/linux-64/zlib-1.2.12-h7f8727e_1.conda>
Elapsed: 00:00.093111
CF-RAY: 740068947daf5b28-IAD

An HTTP error occurred when trying to retrieve this URL.
HTTP errors are often intermittent, and a simple retry will get you on your way.



Exited with code exit status 1
CircleCI received exit code 1

Hope this failure is not related to this PR.

I don't think it's related. @rtburns-jpl to my untrained eye and shallow investigation, it looks like our CircleCI specfile needs to be updated to account for changes here? What do you think? Whatever the case, I'll open a new issue to continue discussion there.

@seongsujeong seongsujeong merged commit 412f083 into isce-framework:main Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants