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

Check DEM provided when extracting layers for which necessary #427

Merged
merged 7 commits into from
Jul 23, 2024

Conversation

rzinke
Copy link
Collaborator

@rzinke rzinke commented Jul 19, 2024

Added function to check that a valid DEM is specified or downloaded when requesting ariaExtract.py to extract 'all' or any of 'bPerpendicular', 'bParallel', 'incidenceAngle', 'lookAngle', 'azimuthAngle', or 'solidEarthTide', for which a DEM is necessary.

@pep8speaks
Copy link

pep8speaks commented Jul 19, 2024

Hello @rzinke! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-07-23 19:08:28 UTC

Additionally, this check was failing originally regardless of what metadata layer was specified as the check for `all` layers
@sssangha
Copy link
Collaborator

sssangha commented Jul 22, 2024

Thanks @rzinke!

I issued a patch to capture a bug with the way the all keyword was handled. In the initial commit, you had dummy-proofed it by forcing it to be lower-case. But the way it was implemented kicked out all keywords that weren't all. I.e. if I specified just SET then an empty list would be passed.

Also, I made some minor tweaks to meet pep8 suggestions. @alexfore , could you please advise on the implementation on this end? Thanks

Here are some simple tests to test this all out:

# get products
wget https://gunw-development-testing.s3.us-west-2.amazonaws.com/unavco_2024/S1-GUNW-A-R-124-tops-20180502_20180408-043026-00157W_00018N-PP-9909-v3_0_1.nc .
wget https://gunw-development-testing.s3.us-west-2.amazonaws.com/unavco_2024/S1-GUNW-A-R-124-tops-20180502_20180408-043052-00157W_00019N-PP-7296-v3_0_1.nc .

# test SET extraction without specifying a valid dem
ariaExtract.py -f "products/*.nc" -l solidEarthTide -w test_set_nodem

# test SET extraction with DEM download specified
ariaExtract.py -f "products/*.nc" -l solidEarthTide -w test_set_dem -d Download

@sssangha sssangha requested a review from alexfore July 22, 2024 19:32
Copy link
Collaborator

@alexfore alexfore left a comment

Choose a reason for hiding this comment

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

I have some quick comments above, should be minor changes.

tools/bin/ariaExtract.py Outdated Show resolved Hide resolved
tools/bin/ariaExtract.py Outdated Show resolved Hide resolved
tools/bin/ariaExtract.py Outdated Show resolved Hide resolved
tools/bin/ariaExtract.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@rzinke rzinke left a comment

Choose a reason for hiding this comment

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

Thank you both. I will implement the suggested changes, except that the ionosphere layer doesn't need a DEM (I checked last week when modifying the notebook).

@rzinke
Copy link
Collaborator Author

rzinke commented Jul 22, 2024

Updates made 0b94a0d
@alexfore could you please review?

Copy link
Collaborator

@alexfore alexfore left a comment

Choose a reason for hiding this comment

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

Thanks!

@sssangha sssangha merged commit 3f17bf5 into aria-tools:dev Jul 23, 2024
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.

4 participants