Fix track number/burst id calculation #77

scottstanie · 2022-10-28T19:40:19Z

This PR implements several fixes to our burst id naming scheme:

For ascending note crossings that happen mid-track, the track number should update (even if there is only one absolute orbit given)
The calculation we've been doing is actually different than ESAs, and leads to a different burst number on many frames (I believe ones closer to the crossing node are more often different since we're missing the "preamble")

For (1), We can double check our work with the manifest.safe file. In a crossing, theres two relativeOrbitNumbers:

(mapping) [staniewi@aurora s1-reader]$ grep -r  relativeOrbitNumber tests/
tests/data/S1A_IW_SLC__1SDV_20221024T184148_20221024T184218_045587_05735F_D6E2.SAFE/manifest.safe:            <safe:relativeOrbitNumber type="start">15</safe:relativeOrbitNumber>
tests/data/S1A_IW_SLC__1SDV_20221024T184148_20221024T184218_045587_05735F_D6E2.SAFE/manifest.safe:            <safe:relativeOrbitNumber type="stop">16</safe:relativeOrbitNumber>

For (2), i'm using the Sentinel-1 Level 1 Detailed Algorithm Definition section 9.25

I've also added 2 test data cases, where I included some scripts to shrink down the real data.

One note: the current test was one where the burst ids we were making were incorrect.
before: expected_burst_id = f't071_{151200 + i}_iw3'

I have added a small sample of the ESA burst database with their geometries. I added a test that compares the overlap to make sure our id matches theirs.

scripts helped make very small version, orbits manually shrunk

in case we add more tests

vbrancat

@scottstanie thanks a lot for this PR. This is so crucial to make the CSLC-S1 workflow to properly work with the TrackFrame database.

My main concern with this PR is related to the unit test. It seems that you are trying to upload a bunch of small file in the repository to make the unit test to run. If I am not mistaken, we decided to avoid going down this route. Our best option would be to store the data on zenodo, download them on the CI, run the unit tests. @LiangJYu and @rtburns-jpl might help you along the way.

If the above is correct, I recommend:

Submit the unit test in a separate PR. You might need some time to set up the repository on zenodo and having the test running on the CI
Include a README file together with the data downloaded for the unit test. This should describe the source of the data, the region covered, what files they contain and why they have been selected for the unit test

vbrancat · 2022-11-17T15:53:43Z

src/s1reader/s1_burst_slc.py

@@ -652,8 +652,9 @@ def eap_compensation_lut(self):
                            f' IPF version = {self.ipf_version}')

        return self.burst_eap.compute_eap_compensation_lut(self.width)
-    def bbox(self):


Any particular reason why we are removing this function?

I removed this once I realized why the border was a list of Polygon's, when it seemed like it was always a list of one single Polygon: opera-adt/burst_db#1

the anti-meridian crossing bursts (international date line) have two polygons, since it's conventional to split the latlon that way, so this function is giving an incorrect bbox for those ones. I can leave it in and submit an issue to correct it in the future if you'd like.

fixing it might be as simple as

return shapely.geometry.MultiPolygon(b.border).bounds

as long as we don't care that it returns longitudes greater than 180:

In [5]: b = s1reader.load_bursts('S1A_IW_SLC__1SDV_20220110T181913_20220110T181938_041402_04EC40_E2D9.zip', None, 1)[0] In [6]: b.border Out[6]: [<shapely.geometry.polygon.Polygon object at 0x7fadef947670>, <shapely.geometry.polygon.Polygon object at 0x7fadef944d00>] In [8]: b.border[0].bounds Out[8]: (180.0, 52.19675210021637, 180.6997402722514, 52.44622930532407) In [11]: import shapely.geometry In [12]: shapely.geometry.MultiPolygon(b.border).bounds Out[12]: (179.3998234765543, 52.19675210021637, 180.6997402722514, 52.50961732430616)

Let's submit an issue separately. We do not want to have longitudes greater than 180 (if this is the only fix)

src/s1reader/s1_reader.py

vbrancat · 2022-11-17T16:03:37Z

src/s1reader/s1_reader.py

@@ -860,9 +850,106 @@ def _burst_from_safe_dir(safe_dir_path: str, id_str: str, orbit_path: str, flag_
    else:
        msg = f'measurement directory NOT found in {safe_dir_path}'
        msg += ', continue with metadata only.'
-        print(msg)
+        # print(msg)


Is it intentional to have a comment here? Don't you want to just print the message for debugging purposes?

ah sorry, I had turned this off while downloading the cycle of metadata so it didn't print the message 3 * 10 * (number of frames in a cycle) times. I'll turn it back on, but it would be nice to have it be a .debug logging message once we switch in python logging or do #25

vbrancat · 2022-11-17T16:28:43Z

src/s1reader/s1_reader.py

+        Relative orbit number at the start of the acquisition, from 1-175.
+    end_track : int
+        Relative orbit number at the end of the acquisition.
+    subswath_name : str, {'IW1', 'IW2', 'IW3'}


Is this case-sensitive?

nope, and I'll add a comment saying so

vbrancat · 2022-11-17T16:29:36Z

src/s1reader/s1_reader.py

+    ESA Sentinel-1 Level 1 Detailed Algorithm Definition
+    https://sentinels.copernicus.eu/documents/247904/1877131/S1-TN-MDA-52-7445_Sentinel-1+Level+1+Detailed+Algorithm+Definition_v2-4.pdf/83624863-6429-cfb8-2371-5c5ca82907b8
+    """
+    # Constants in Table 9-7


Wondering again whether we should have a separate file in the repository where to insert all the constants

Same comment here- I can move these to a constants file if you'd like, but i'm unsure if the constants would be easier to interpret if they're moved away from function that uses them. If we're planning on making a big constants.py then I can start with these

I don't think there's enough for a dedicated constants file. From a quick glance, I didn't see anything beyond 3 constants below in the repo.

What do you about moving get_burst_id into its own file? A s1_burst_id.py that's something like the following (some inspiration from):

from typing import ClassVar from dataclasses import dataclass @dataclass class S1BurstID: T_beam: ClassVar[float] = 2.758273 # interval of one burst [s] T_pre: ClassVar[float] = 2.299849 # Preamble time interval [s] T_orb: ClassVar[float] = 12 * 24 * 3600 / 175 # Nominal orbit period [s] track_number: int esa_burst_id: int subswath_name: str @classmethod def params_to_id(cls, sensing_time: datetime.datetime, ascending_node_dt: datetime.datetime, start_track: int, end_track: int, subswath_name: str): # do same computations return cls(track_numer, esa_burst_id, subswath) @property def as_str(self): return f"t{self.track_number:03d}_{self.esa_burst_id:06d}_{self.subswath_name.lower()}"

We can then change this from str to S1BurstID

Benefits:

By importing this module, all the constants as accessible a class properties.

Clearer way to import and generate JPL S1 burst IDs without having to dig through s1_reader.py.

vbrancat · 2022-11-17T16:37:17Z

tests/data/README.md

@@ -0,0 +1,33 @@
+- `make_empty_safe.sf` converts a full SDV SAFE folder into a 2.1 Mb folder of 1 vv-polarization annotation files.
+- `make_empty_img.py` Converts a measurement/ .tiff file into an all-zeros file < 50kb using SPARSE_OK=TRUE.


Not sure if it is intentional, but I do see that this PR is trying to upload a lot of files into the repository. @LiangJYu can correct me if I am wrong, but I thought we decided to upload all the test data on zenodo.org in order to avoid increasing the size of the s1-reader repo. If this is the case, I would recommend to submit the unit test in a separate PR as this would require some more work (i.e. data uploading, set the CI) and we desperately need this PR for our CSLC-S1 Beta delivery

…t/end track

scottstanie · 2022-11-18T00:44:40Z

MANIFEST.in

@@ -1 +1 @@
-include src/s1reader/data/sentinel1_track_burst_id.txt
+recursive-include src/s1reader/data/ *


As a note: this will fix #85 by including all the files in the data folder

scottstanie · 2022-11-18T00:48:22Z

alright @vbrancat now this PR is much smaller haha. i've separated out the unit tests/new test file generation into #84 and i'll talk with @LiangJYu about his usage of Zenodo-pulled files.

seongsujeong · 2022-11-18T00:35:23Z

tests/create_esa_db_sample.py

@@ -0,0 +1,47 @@
+import pandas as pd


Not sure if pandas is included in the dependency for s1-reader?

ok i'll keep that in mind for the other PR that I moved these test scripts to 👍

seongsujeong · 2022-11-18T00:45:32Z

src/s1reader/s1_reader.py

+    has_anx_crossing = (end_track == start_track + 1) or (
+        end_track == 1 and start_track == 175
+    )


Suggested change

has_anx_crossing = (end_track == start_track + 1) or (

end_track == 1 and start_track == 175

)

has_anx_crossing = (end_track == (start_track + 1) % 175)

haha I think i waffled back and forth between whether the modulo was clearer or the other one. i'll change to the modulo version

(idk why i changed it back from this commit https://github.com/opera-adt/s1-reader/blob/dba751c6247481ebc5531ffaf11566bd03a25d34/src/s1reader/s1_reader.py#L890 )

seongsujeong · 2022-11-18T00:58:15Z

src/s1reader/s1_reader.py

+    -------
+    str
+        Search path to extract from the ET of the manifest.safe XML.
+


Nice improvement of the function!
Please add description for the returning namespace nsmap

good idea, i added a comment and a link to the lxml document where it came from

src/s1reader/s1_reader.py

LiangJYu · 2022-11-18T17:30:57Z

src/s1reader/s1_reader.py

+    ESA Sentinel-1 Level 1 Detailed Algorithm Definition
+    https://sentinels.copernicus.eu/documents/247904/1877131/S1-TN-MDA-52-7445_Sentinel-1+Level+1+Detailed+Algorithm+Definition_v2-4.pdf/83624863-6429-cfb8-2371-5c5ca82907b8
+    """
+    # Constants in Table 9-7


I don't think there's enough for a dedicated constants file. From a quick glance, I didn't see anything beyond 3 constants below in the repo.

What do you about moving get_burst_id into its own file? A s1_burst_id.py that's something like the following (some inspiration from):

from typing import ClassVar from dataclasses import dataclass @dataclass class S1BurstID: T_beam: ClassVar[float] = 2.758273 # interval of one burst [s] T_pre: ClassVar[float] = 2.299849 # Preamble time interval [s] T_orb: ClassVar[float] = 12 * 24 * 3600 / 175 # Nominal orbit period [s] track_number: int esa_burst_id: int subswath_name: str @classmethod def params_to_id(cls, sensing_time: datetime.datetime, ascending_node_dt: datetime.datetime, start_track: int, end_track: int, subswath_name: str): # do same computations return cls(track_numer, esa_burst_id, subswath) @property def as_str(self): return f"t{self.track_number:03d}_{self.esa_burst_id:06d}_{self.subswath_name.lower()}"

We can then change this from str to S1BurstID

Benefits:

By importing this module, all the constants as accessible a class properties.

Clearer way to import and generate JPL S1 burst IDs without having to dig through s1_reader.py.

scottstanie · 2022-11-21T18:18:57Z

@LiangJYu the new module is up, where you make the S1BurstId with the .from_burst_params classmethod

we'll have to change this bit in COMPASS

https://github.com/opera-adt/COMPASS/blob/bb2630675878d8a227e1c0eeb0ea1dd0266c378e/src/compass/utils/runconfig.py#L395-L400

to '_'.join(str(b.burst_id)) to join the burst id to other strings

LiangJYu

LGTM. Really nice changes/additions to s1-reader!

src/s1reader/s1_reader.py

scottstanie · 2022-11-21T19:40:05Z

also, perhaps I should open an issue in RTC for @gshiroma for the same small fix once this is merged in: https://github.com/opera-adt/RTC/blob/93c3118ea4cb91058c4afc0a9e410444b038a57f/src/rtc/runconfig.py#L428 which will just have to coerce to str() to combine using join()

scottstanie · 2022-11-21T19:44:04Z

although I actually see one other change to the RTC repo: https://github.com/opera-adt/RTC/blob/cf04dc7ff68f559d3fa56605321c312a144b3e7b/src/rtc/h5_prep.py#L144

@LiangJYu do you think

we should add a method to S1BurstId that matches the string.split method

def split(sep=None, maxsplit=-1):
    return str(self).split(sep, maxsplit)

we should have people explicitly do str(burst_id_obj) to use the string methods on the string repr of S1BurstId?

LiangJYu · 2022-11-21T20:17:57Z

although I actually see one other change to the RTC repo: https://github.com/opera-adt/RTC/blob/cf04dc7ff68f559d3fa56605321c312a144b3e7b/src/rtc/h5_prep.py#L144

@LiangJYu do you think
1. we should add a method to `S1BurstId` that matches the string.split method
def split(sep=None, maxsplit=-1):
    return str(self).split(sep, maxsplit)
2. we should have people explicitly do `str(burst_id_obj)` to use the string methods on the string repr of S1BurstId?

With regards to the RTC code, I think it would be clearer if it track_number was retrieved via burst_in.burst_id.track_number instead of string.split(). This train of thought sort of leads to me thinking we should not add a split method.

What other string methods were you think about besides split? Comparisons for individual attributes?

scottstanie · 2022-11-21T20:38:09Z

i didn't have any others; I was just checking thoughts on trying to not break other code now that are expected it to be a string, vs. having everyone switch to use the new object

gshiroma · 2022-11-22T19:13:08Z

i didn't have any others; I was just checking thoughts on trying to not break other code now that are expected it to be a string, vs. having everyone switch to use the new object

Thank you, @scottstanie and @LiangJYu ! No problem if you guys decide to go ahead and make burst_id an object (other than str). We'll update our code accordingly. The fact that you @scottstanie identified the points of change and @LiangJYu suggested a solution makes it even easier to fix. Thanks again!

vbrancat

@scottstanie Thanks for having addressed my comments. I have no comment left at this stage :). Nice work.

scottstanie added 2 commits October 28, 2022 11:50

add more small testing data

58c0648

scripts helped make very small version, orbits manually shrunk

make a failing test for anx crossing

eb8b2f3

scottstanie marked this pull request as draft October 28, 2022 19:40

scottstanie added 5 commits October 28, 2022 16:48

create methods for burst/track, fill tests

556e44d

fix erroneous burst id in existing test

42df314

add a sample burst db to compare

63859d8

add script for remaking the burst sample

2c904d5

in case we add more tests

add a geometry check for the esa database

af6d4ca

scottstanie marked this pull request as ready for review October 29, 2022 00:41

scottstanie added 3 commits October 28, 2022 17:44

perform test without pandas

53dc5ea

codacy items

0f86d66

add the geometry comparison to the other test cases

2ed1623

scottstanie requested review from LiangJYu, vbrancat, yunjunz and hfattahi October 29, 2022 01:03

scottstanie added 9 commits October 31, 2022 17:18

add two more test cases

cbabae1

refactor tests for new cases

98eb819

redo logic for strange track175 case

dba751c

update burst csv

2daef08

fix first test problem, cut bursts

a1e13c4

get tests working again for track175 case

2236cf7

fix esa db csv

7e95f3c

use nsmap instead of long manual urls

2382df4

remove testing script

9e1b544

scottstanie marked this pull request as draft November 1, 2022 16:38

scottstanie added 2 commits November 1, 2022 13:33

working version on full orbit cycle

1c1dc57

fix tests to check all subswaths

5d19f7d

scottstanie marked this pull request as ready for review November 1, 2022 20:52

try recursive include for circleci fail, codacy

fcf833d

vbrancat reviewed Nov 17, 2022

View reviewed changes

scottstanie requested a review from seongsujeong November 17, 2022 23:52

scottstanie added 4 commits November 17, 2022 19:30

add better docstrings, add print missing tiff again, comment for star…

23c02d4

…t/end track

case sensitivity clarification

e3cd8be

revert tests folder back to current version

c2b8602

fix unit test for correct burst id

a43a60b

scottstanie commented Nov 18, 2022

View reviewed changes

seongsujeong reviewed Nov 18, 2022

View reviewed changes

scottstanie added 2 commits November 17, 2022 20:08

Merge branch 'main' into burst-id-fix

0fa41e5

adjust anx crossing to use mod, add comment on lxml nsmap

cf85764

LiangJYu reviewed Nov 18, 2022

View reviewed changes

scottstanie added 5 commits November 21, 2022 12:26

split out burst id into class

84888ce

fix burst load filtering for new class

6565058

use the class vars in initialization

7cc6150

formatting

ddd1dc1

just use __str__ instead of as_str, adjust as_dict

b227f84

LiangJYu approved these changes Nov 21, 2022

View reviewed changes

src/s1reader/s1_reader.py Outdated Show resolved Hide resolved

src/s1reader/s1_reader.py Show resolved Hide resolved

src/s1reader/s1_reader.py Show resolved Hide resolved

address comments for clarity

2db91b1

vbrancat approved these changes Nov 22, 2022

View reviewed changes

scottstanie merged commit d8f9e80 into isce-framework:main Nov 23, 2022

scottstanie deleted the burst-id-fix branch November 23, 2022 12:30

yunjunz mentioned this pull request Dec 2, 2022

bugfix in Sentinel1BurstSlc.swath_name() #89

Merged

scottstanie mentioned this pull request Oct 13, 2023

[Bug]: AUX_CAL data files aren't included in a pip install #85

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix track number/burst id calculation #77

Fix track number/burst id calculation #77

scottstanie commented Oct 28, 2022 •

edited

Loading

vbrancat left a comment

vbrancat Nov 17, 2022

scottstanie Nov 17, 2022

scottstanie Nov 18, 2022

vbrancat Nov 22, 2022

vbrancat Nov 17, 2022

scottstanie Nov 18, 2022

vbrancat Nov 17, 2022

scottstanie Nov 18, 2022

vbrancat Nov 17, 2022

scottstanie Nov 18, 2022

LiangJYu Nov 18, 2022 •

edited

Loading

vbrancat Nov 17, 2022

scottstanie Nov 18, 2022

scottstanie commented Nov 18, 2022

seongsujeong Nov 18, 2022

scottstanie Nov 18, 2022

seongsujeong Nov 18, 2022

scottstanie Nov 18, 2022

scottstanie Nov 18, 2022

seongsujeong Nov 18, 2022

scottstanie Nov 18, 2022

LiangJYu Nov 18, 2022 •

edited

Loading

scottstanie commented Nov 21, 2022

LiangJYu left a comment

scottstanie commented Nov 21, 2022

scottstanie commented Nov 21, 2022

LiangJYu commented Nov 21, 2022

scottstanie commented Nov 21, 2022

gshiroma commented Nov 22, 2022

vbrancat left a comment

		@@ -0,0 +1,33 @@
		- `make_empty_safe.sf` converts a full SDV SAFE folder into a 2.1 Mb folder of 1 vv-polarization annotation files.
		- `make_empty_img.py` Converts a measurement/ .tiff file into an all-zeros file < 50kb using SPARSE_OK=TRUE.

		@@ -1 +1 @@
		include src/s1reader/data/sentinel1_track_burst_id.txt
		recursive-include src/s1reader/data/ *

Fix track number/burst id calculation #77

Fix track number/burst id calculation #77

Conversation

scottstanie commented Oct 28, 2022 • edited Loading

vbrancat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiangJYu Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottstanie commented Nov 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiangJYu Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

scottstanie commented Nov 21, 2022

LiangJYu left a comment

Choose a reason for hiding this comment

scottstanie commented Nov 21, 2022

scottstanie commented Nov 21, 2022

LiangJYu commented Nov 21, 2022

scottstanie commented Nov 21, 2022

gshiroma commented Nov 22, 2022

vbrancat left a comment

Choose a reason for hiding this comment

scottstanie commented Oct 28, 2022 •

edited

Loading

LiangJYu Nov 18, 2022 •

edited

Loading

LiangJYu Nov 18, 2022 •

edited

Loading