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

Use just the frame-id for CLI #92

Merged
merged 20 commits into from
Feb 6, 2023
Merged

Use just the frame-id for CLI #92

merged 20 commits into from
Feb 6, 2023

Conversation

cmarshak
Copy link
Collaborator

@cmarshak cmarshak commented Jan 30, 2023

Per @asjohnston-asf suggestion, utilize just the frame-id to submit jobs.

I am reading directly from an s3 bucket. We could also add the dependency from s1-frame-enumerator and read it from that. Not sure about egress implications - should be infinitesimal.

raise RuntimeError('Region of interest does not overlap with IFG '
'area (ref and sec overlap)')
if frame_id != -1:
df_frames = gpd.read_file('s3://s1-gunw-frames/s1_frames.geojson',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is only accessible using JPL A19 AWS credentials and won't be accessible from other deployments, or to developers locally (without effort). If you're going to keep it in an S3 bucket, it'd be best to make it a public bucket.

However, since this file is only 72 mb, and compresses down to 22 mb with zip, I think you'd be best off just including it in the repo. That way, it's guaranteed to work for local developers, in the container, and in any deployment. Also, that way changes to the frames are explicitly captured in PRs/git history, and documented in the changelog.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding the enumerator as a dependency and reading from that would also work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am going to add the enumerator as a dependency. There are issues with the frame size/overlap as it takes too long. Will get clarity soon from @dbekaert.

@cmarshak
Copy link
Collaborator Author

cmarshak commented Feb 1, 2023

Relevant discussion (shared this with @asjohnston-asf yesterday)

Bug: #93
Discussion: #94

tldr: we need to figure out proper size of frames - this is going to be very fluid implementation for a few weeks.

@cmarshak
Copy link
Collaborator Author

cmarshak commented Feb 3, 2023

@jhkennedy and @asjohnston-asf

This is finally ready for review and to be used to update hyp3.

Notes:

  1. Included frame data as zip file in this repository. Will be easiest currently because the enumerator is really 3.10+ and there are no tests yet for this.
  2. Included a bunch of integration tests that can be easily ported to unit tests once there is a bit more time to use the examples Andrew shared with me earlier this week. Do a bunch of checks to make sure we are submitting correct pairs
  3. Had to pin ISCE2 to version 2.6.1 (the previous version) with numpy/scipy pinned too until some build issues are sorted out. We will likely want to figure out a better way to increment version numbers for ISCE2 since the errors are pretty cryptic. Thanks, @sssangha.
  4. Most importantly, frame id CLI is there. See the README for examples. We are using 8 bursts across all 3 swaths with a guarantee of 1 burst overlap (usually more because we are using a bbox to specify processing area in ISCE2).

Here is the GUNW for the first example captured in the readme [link]. For additional context, here are some geojsons of the frame 22438 and its bounding box.

Copy link
Collaborator

@jhkennedy jhkennedy left a comment

Choose a reason for hiding this comment

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

I think this looks good now; but I wasn't able to open the NetCDF you posed because both QGIS and ncview give me an error like:

ncview: can't recognize format of input file S1-GUNW-D-R-144-tops-20230125_20221220-140045-00121W_00035N-PP-4569-v2_0_6.nc

@jhkennedy
Copy link
Collaborator

That is, in terms of testing the hyp3 integration, this looks good to me. But the actual product produced likely needs work.

@cmarshak
Copy link
Collaborator Author

cmarshak commented Feb 6, 2023

The netcdf thing doesn't suprise me. I get similar errors with xarray. We have to use h5py to open GUNWs.

I can view the product in QGIS (or at least certain rasters) using the QGIS LTR (long-term release). The newest versions of QGIS always fail.

image

image

@cmarshak cmarshak merged commit d0bf5d7 into dev Feb 6, 2023
@cmarshak cmarshak deleted the remove-bbox-and-use-frame-id branch February 6, 2023 20:02
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.

2 participants