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

Add capability to created wanted variables list with an explicit list of paths #508

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

Conversation

andypbarrett
Copy link

This pull request adds the capability to the Variables.append method to create wanted variables using a list of explicit paths.

  • a path_list keyword argument was added to append.
  • parse_var_list and set are used to generate unique var_list, beam_list, keyword_list variables. These are then used as they had been in the original version of append.
  • the docstring for append was updated to describe the new keyword and give a usage example.
  • keyword checking is added to ensure path_list but not var_list, beam_list, or keyword_list are set. A ValueError is raised if this check fails.
  • The check that at least one combination of keywords is set was updated to include path_list and is modified to use if instead of assert. This allows a more informative Exception to be raised than an AssertionError

Copy link

github-actions bot commented Feb 9, 2024

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

I will automatically update this comment whenever this PR is modified

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

Binder 👈 Launch a binder notebook on this branch for commit 4dd740b

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

Binder 👈 Launch a binder notebook on this branch for commit 4054e1f

Binder 👈 Launch a binder notebook on this branch for commit 4824bd4

Binder 👈 Launch a binder notebook on this branch for commit 1d868bd

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

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.

Looks good to me! Thanks, @andypbarrett (especially for updating the docstring with an example, too!)

@JessicaS11 JessicaS11 linked an issue Feb 13, 2024 that may be closed by this pull request
@JessicaS11
Copy link
Member

Thanks for the great PR, @andypbarrett! My comments are mostly typos, and hopefully a fix to get the tests via Travis to pass.

@andypbarrett
Copy link
Author

Thanks @JessicaS11 and @rwegener2 . I plan on fixing these on Thursday 15 Feb.

andypbarrett and others added 3 commits February 15, 2024 16:42
Co-authored-by: Jessica Scheick <[email protected]>
Co-authored-by: Jessica Scheick <[email protected]>
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

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

Project coverage is 66.02%. Comparing base (de13727) to head (e294e31).
Report is 1 commits behind head on development.

Files Patch % Lines
icepyx/core/variables.py 10.00% 9 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Comment on lines +533 to +535
var_list = list(set(variables))
beam_list = list(set(beams))
keyword_list = list(set(keywords))
Copy link
Member

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:

  1. 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)
  2. 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.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

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.

Allow generating variables wanted from a list of h5 paths
3 participants