isce-framework / s1-reader

Sentinel-1 reader
Apache License 2.0
27 stars 12 forks source link

Option to concatenate RESORB file #133

Closed seongsujeong closed 1 year ago

seongsujeong commented 1 year ago

This PR, built upon @scottstanie's PR #132, attempts to solve the issue that can take place when using RESORB file with ascending node crossing (ANX) time correction.

RESORB file, which will be the major orbit information source for forward processing for OPERA, has shorter time range than POEORB file. It is about as long as two times of the orbit period (T_orb = 1:38:44.57), with time shifting of about T_orb. That means, In rare case, this can cause that no single RESORB file can cover the time range of [sensing_start - T_orb, sensing_stop], which is necessary for ascending node crossing time correction. This PR attempts to find the two RESORB files, and concatenate them into one. The two source RESORB files are:

The concatenation takes place either when trying to find the orbit files from directory (get_orbit_file_from_dir()), or when downloading the orbit file (retrieve_orbit_file(), which was download_orbit())

seongsujeong commented 1 year ago

Thank you @scottstanie and @gshiroma for taking a look at the PR and making a good points in the comment. Would you mind taking another look? Thanks.

seongsujeong commented 1 year ago

Last question-

In get_orbit_dict, the arguments are start_time, end_time, which each get padding added to them. We're doing other specific padding to the start/end elsewhere in the code depending on the orbit type. Should we get rid of this?

    # Add a 30 min margin to start_time and end_time
    pad_30_min = datetime.timedelta(hours=0.5)
    pad_start_time = start_time - pad_30_min
    pad_end_time = end_time + pad_30_min

based on my cartoon-based thought, 2 * 30 mins of padding would be okay for timing design of RESORB (duration, time shifting). However, shorter padding might help reducing the chance of the unnecessary use of RESORB pair. Based on the personal conversation with @vbrancat, coordination with SDS (about the policy change), and time constrain we have, I prefer not to touch this part.

scottstanie commented 1 year ago

Sure not changing it now due to the time crunch is fine. I just wanted to make sure we know that we are padding the start once with T_ORBIT + 60 seconds and then again with 30 minutes in a different function

seongsujeong commented 1 year ago

TODO: consider get rid of 30 min padding in get_orbit_dict()

edit: DONE

seongsujeong commented 1 year ago

@scottstanie @hfattahi @gshiroma Thank you for the good comments and feedback. I think this PR is ready for the another round of review.

seongsujeong commented 1 year ago

@LiangJYu Thanks for the feedback. All good points. Please let me know if you have more suggestions, or if I have missed responding some of your comments. Thank you!

seongsujeong commented 1 year ago

Thank you @scottstanie, @gshiroma, @hfattahi, and @LiangJYu for the review! I think the code is much better than the initial commit.