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

Testing ATL14 gridded land ice download from earthdata #289

Open
wants to merge 14 commits into
base: development
Choose a base branch
from

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented Mar 6, 2022

Adding unit tests for ATL14 data access via Earthdata, using a region over Svalbard (Iceland is smaller, but bounding box overlaps with Greenland which is big).

Towards resolving #288, but also updating tests behind NSIDC login for the ATL06 dataset by updating from v05 to v06.

@weiji14 weiji14 self-assigned this Mar 6, 2022
@github-actions
Copy link

github-actions bot commented Mar 6, 2022

Binder 👈 Launch a binder notebook on this branch for commit 2a84dfb

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit 2feaf5d

Binder 👈 Launch a binder notebook on this branch for commit 65cfdac

Binder 👈 Launch a binder notebook on this branch for commit 558c2c4

Binder 👈 Launch a binder notebook on this branch for commit f4b9413

Binder 👈 Launch a binder notebook on this branch for commit 8010274

Binder 👈 Launch a binder notebook on this branch for commit 61f5fcd

Binder 👈 Launch a binder notebook on this branch for commit d4bc85f

Binder 👈 Launch a binder notebook on this branch for commit ba7b4f3

Binder 👈 Launch a binder notebook on this branch for commit a813fa4

Binder 👈 Launch a binder notebook on this branch for commit a326c8d

Binder 👈 Launch a binder notebook on this branch for commit 538729b

@codecov-commenter
Copy link

codecov-commenter commented Mar 6, 2022

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 66.22%. Comparing base (b2dbb14) to head (538729b).

Files Patch % Lines
icepyx/tests/test_behind_NSIDC_API_login.py 0.00% 6 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           development     #289      +/-   ##
===============================================
+ Coverage        66.19%   66.22%   +0.02%     
===============================================
  Files               36       36              
  Lines             3065     3064       -1     
  Branches           541      540       -1     
===============================================
  Hits              2029     2029              
+ Misses             945      944       -1     
  Partials            91       91              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JessicaS11
Copy link
Member

Looks like the issue with adding this test is now #286.

@weiji14
Copy link
Member Author

weiji14 commented Mar 7, 2022

Ok, but I might as well add some unit tests for ATL14 since I've been playing with it recently. Will see what I can do to increase code coverage.

@JessicaS11
Copy link
Member

Ok, but I might as well add some unit tests for ATL14 since I've been playing with it recently. Will see what I can do to increase code coverage.

Sounds great (and wasn't suggesting we not add the tests already here). Just noticed that now that #288 is resolved with updated certificates at NSIDC, the failing Travis errors were "updated" to #286.

@JessicaS11
Copy link
Member

See #291

weiji14 added 6 commits March 7, 2022 23:17
Not using Iceland's spatial extent, because it overlaps with the Greenland bounding box and causes the unit test to download >1GB of data.
Easier to debug what specific tests went wrong
Loop through each key one at a time, so that it's easier to debug when keys are missing.
Created using `json.dump(obj=obs, fp=open("ATL06v06_options.json", "w"))`.
@weiji14 weiji14 marked this pull request as ready for review August 8, 2023 19:10
Copy link
Member Author

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Cool, should be ready for review! Have updated the test_behind_NSIDC_API_login to include the new ATL14 (v002) dataset, and also updated the ATL06 schema to v006 for good measure.

icepyx/tests/ATL06v06_options.json Outdated Show resolved Hide resolved
@weiji14 weiji14 requested a review from rwegener2 August 31, 2023 00:00
@weiji14
Copy link
Member Author

weiji14 commented Aug 31, 2023

Hi @rwegener2, could you give this a review if you have time, since you recently worked on the #435 PR?

@rwegener2
Copy link
Contributor

@rwegener2, could you give this a review if you have time

Absolutely, @weiji14. I've been offline for a day or two while moving apartments, but I'll take a look this afternoon.

Copy link
Contributor

@rwegener2 rwegener2 left a comment

Choose a reason for hiding this comment

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

This looks good to me @weiji14. Nothing really changed with respect to authentication as far as I can tell, was there something specific you wanted a second set of eyes on? The ATL14/06 specific bits I am less familiar with, but from my experience everything looks solid!

Copy link
Member

Choose a reason for hiding this comment

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

@weiji14 If I'm understanding this right, we've added ATL14 as part of the fixture, but then don't actually run any tests on it beyond creating a query object with those parameters (in which case, shouldn't we just create a query object with it elsewhere instead of in test_behind_NSIDC_API_login.py). That or the assertions in lines 50-51 will fail because there's no appropriate json file for the ATL14 custom options...

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