-
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
Add capability to created wanted variables list with an explicit list of paths #508
base: development
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
👈 Launch a binder notebook on this branch for commit 1342525 I will automatically update this comment whenever this PR is modified 👈 Launch a binder notebook on this branch for commit acb5a52 👈 Launch a binder notebook on this branch for commit 4dd740b 👈 Launch a binder notebook on this branch for commit 7603309 👈 Launch a binder notebook on this branch for commit 4054e1f 👈 Launch a binder notebook on this branch for commit 4824bd4 👈 Launch a binder notebook on this branch for commit 1d868bd 👈 Launch a binder notebook on this branch for commit e294e31 |
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.
Looks good to me! Thanks, @andypbarrett (especially for updating the docstring with an example, too!)
Thanks for the great PR, @andypbarrett! My comments are mostly typos, and hopefully a fix to get the tests via Travis to pass. |
Thanks @JessicaS11 and @rwegener2 . I plan on fixing these on Thursday 15 Feb. |
Co-authored-by: Jessica Scheick <[email protected]>
Co-authored-by: Jessica Scheick <[email protected]>
Co-authored-by: Jessica Scheick <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #508 +/- ##
===============================================
- Coverage 66.19% 66.02% -0.18%
===============================================
Files 36 36
Lines 3065 3073 +8
Branches 541 544 +3
===============================================
Hits 2029 2029
- Misses 945 953 +8
Partials 91 91 ☔ View full report in Codecov by Sentry. |
var_list = list(set(variables)) | ||
beam_list = list(set(beams)) | ||
keyword_list = list(set(keywords)) |
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.
I went to add the new example in the docstring to the variables tutorial notebook it raised 2 issues:
- The example fails because it's for ATL03, and the examples are on ATL06 (and in the notebook, there's a second one for ATL09)
- Trying to use a
path_list
input on ATL06 fails because it's got four layers of variables, not the three we collect here.
I haven't reminded myself of how we handle this in general in the module, so that may recommend a solution for (2). (1) is easy if we just sub in another list.
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
This pull request adds the capability to the
Variables.append
method to create wanted variables using a list of explicit paths.path_list
keyword argument was added toappend
.parse_var_list
andset
are used to generate uniquevar_list
,beam_list
,keyword_list
variables. These are then used as they had been in the original version of append.append
was updated to describe the new keyword and give a usage example.var_list
,beam_list
, orkeyword_list
are set. AValueError
is raised if this check fails.path_list
and is modified to useif
instead ofassert
. This allows a more informative Exception to be raised than anAssertionError