Skip to content

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

Open
andypbarrett wants to merge 15 commits intodevelopmentfrom
add-variable-path-list
Open

Add capability to created wanted variables list with an explicit list of paths#508
andypbarrett wants to merge 15 commits intodevelopmentfrom
add-variable-path-list

Conversation

@andypbarrett
Copy link
Copy Markdown

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

@github-actions
Copy link
Copy Markdown

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
Copy Markdown
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!)

Comment thread icepyx/core/variables.py Outdated
Comment thread icepyx/core/variables.py Outdated
Comment thread icepyx/core/variables.py Outdated
@JessicaS11 JessicaS11 linked an issue Feb 13, 2024 that may be closed by this pull request
Comment thread icepyx/core/variables.py Outdated
@JessicaS11
Copy link
Copy Markdown
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
Copy Markdown
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 <JessicaS11@users.noreply.github.com>
Co-authored-by: Jessica Scheick <JessicaS11@users.noreply.github.com>
Co-authored-by: Jessica Scheick <JessicaS11@users.noreply.github.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 15, 2024

Codecov Report

❌ Patch coverage is 10.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.02%. Comparing base (de13727) to head (e294e31).
⚠️ Report is 134 commits behind head on development.

Files with missing lines 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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread icepyx/core/variables.py
Comment on lines +533 to +535
var_list = list(set(variables))
beam_list = list(set(beams))
keyword_list = list(set(keywords))
Copy link
Copy Markdown
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.

@review-notebook-app
Copy link
Copy Markdown

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