-
Notifications
You must be signed in to change notification settings - Fork 107
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
base: development
Are you sure you want to change the base?
Conversation
👈 Launch a binder notebook on this branch for commit 2a84dfb I will automatically update this comment whenever this PR is modified 👈 Launch a binder notebook on this branch for commit 2feaf5d 👈 Launch a binder notebook on this branch for commit 65cfdac 👈 Launch a binder notebook on this branch for commit 558c2c4 👈 Launch a binder notebook on this branch for commit f4b9413 👈 Launch a binder notebook on this branch for commit 8010274 👈 Launch a binder notebook on this branch for commit 61f5fcd 👈 Launch a binder notebook on this branch for commit d4bc85f 👈 Launch a binder notebook on this branch for commit ba7b4f3 👈 Launch a binder notebook on this branch for commit a813fa4 👈 Launch a binder notebook on this branch for commit a326c8d 👈 Launch a binder notebook on this branch for commit 538729b |
Codecov ReportAttention: Patch coverage is
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. |
Looks like the issue with adding this test is now #286. |
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. |
See #291 |
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"))`.
There was a problem hiding this 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.
Shouldn't be needed since $NSIDC_LOGIN is already set?
…ble" This reverts commit a813fa4.
Hi @rwegener2, could you give this a review if you have time, since you recently worked on the #435 PR? |
Absolutely, @weiji14. I've been offline for a day or two while moving apartments, but I'll take a look this afternoon. |
There was a problem hiding this 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!
There was a problem hiding this comment.
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...
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.