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.

Sign up for GitHub

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

Jump to bottom

Update filter_by_attrs to use 'variables' instead of 'data_vars' #3247

Merged
merged 5 commits into from
Aug 23, 2019
Merged

Update filter_by_attrs to use 'variables' instead of 'data_vars' #3247

merged 5 commits into from
Aug 23, 2019

Conversation

cspencerjones
Copy link
Contributor

This will allow filter_by_attrs to filter coordinates as well as variables. @ocefpaf

@cspencerjones
Update filter_by_attrs to use 'variables' instead of 'data_vars'
7ebdfc1 
This will allow `filter_by_attrs` to filter coordinates as well as variables. @ocefpaf
@dcherian
Copy link
Contributor

Thanks @cspencerjones . Can you add a test that tests this?

@ocefpaf
Copy link
Contributor

ocefpaf commented Aug 23, 2019

Thanks @cspencerjones . Can you add a test that tests this?

I guess we can just extend the existing test and check if we can find a coordinate by its attributes. For example, add a standard name for time here and see if we can find time using it.

@dcherian
Copy link
Contributor

Exactly.

@dopplershift
Copy link
Contributor

cc: @jthielen Might make some things in MetPy easier...

@pep8speaks
Copy link

pep8speaks commented Aug 23, 2019 •
edited
Loading

Hello @cspencerjones! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-08-23 19:03:46 UTC

jthielen
jthielen suggested changes Aug 23, 2019
View reviewed changes
Copy link
Contributor

@jthielen jthielen left a comment

Choose a reason for hiding this comment

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

This will be great to have! I just noticed something relating to CF compliance:

xarray/tests/test_dataset.py Outdated Show resolved Hide resolved
xarray/tests/test_dataset.py Outdated Show resolved Hide resolved
xarray/tests/test_dataset.py Outdated Show resolved Hide resolved
@max-sixty
Copy link
Collaborator

Thanks @cspencerjones ! Great to have you as a contributor

@dcherian
Copy link
Contributor

Thanks @cspencerjones

@dcherian dcherian merged commit 011f7cc into pydata:master Aug 23, 2019
@dcherian
Copy link
Contributor

Oh no! I forgot to ask you to add a note in whats-new.rst. Can you send in a new PR with a small note in that file please?

dcherian added a commit to dcherian/xarray that referenced this pull request Aug 26, 2019
@dcherian
Merge remote-tracking branch 'upstream/master' into sparse-fixes
e114cad 
* upstream/master:
  Initialize empty or full DataArray (pydata#3159)
  Raise on inplace=True (pydata#3260)
  added support for rasterio geotiff tags (pydata#3249)
  Remove sel_points (pydata#3261)
  Fix sparse ops that were calling bottleneck (pydata#3254)
  New feature of filter_by_attrs added (pydata#3259)
  Update filter_by_attrs to use 'variables' instead of 'data_vars' (pydata#3247)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jthielen jthielen jthielen requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@cspencerjones @dcherian @ocefpaf @dopplershift @pep8speaks @max-sixty @jthielen