issue_comments

OK, in it goes. Thanks @ocefpaf !

:+1: for filter_by_attrs , and I find it good to have such convenience functions in addition to the general ones like https://github.com/pydata/xarray/issues/916 : newcomers will find their way through xarray more easily

Anyone else have an opinion on the name? I'd like to merge this shortly before releasing v0.8.0 tomorrow.

Done. Not sure why only AppVeyor started :confused:

Assuming we do add the filter method, maybe filter_by_attrs?

Makes sense. I will modify this soon.

Assuming we do add the filter method, maybe filter_by_attrs?

It's nice to use names with the same prefix because they appear next to each other in auto-complete.

I do think the name is a mouthful, though :).

I agree that is a mouthful but then again, I was trying to be consistent with the existing versions. However, I don't really have a strong opinion on the matter and I am fine with whatever you decide. Should I renamed then? filter_attrs? (Sounds weird though b/c you are not filtering the attrs, but the variables based on the attrs :expressionless:)

I'm OK with the specialized get_variables_by_attributes. It doesn't need to be mutually exclusive with the more generic option. I do think the name is a mouthful, though :).

I fine with whatever you decide but here are my two cents: - get_variables_by_attributes is the same name of this method in netcd4 and some java netcdf libraries. So I'd rather not have a specialized version for attributes than adding it with name with a different name. - I see the elegance in s.filter(lambda x: x.attrs['standard_name'] == 'convective_precipitation_flux') and I like it a lot! But the specialized version for attributes is more compact to write and, at least in my field, filtering by attributes is more common making this version more convenient.

Feel free to close this if you don't think it is worth adding the specialized version. Or let me know if you want to rename it.

@ocefpaf Good point. It probably is indeed better to write be able to write something generic like ds.filter(lambda x: x.attrs['standard_name'] == 'convective_precipitation_flux') rather the specialized to attributes ds.get_variables_by_attributes(standard_name='convective_precipitation_flux').

Another option would be simply renaming this to filter_attrs, which is more succinct.

Is there still an interested in this or should I close in light of https://github.com/pydata/xarray/issues/883?

Agreed -- this is almost ready. Please add to the API docs (api.rst) and do the docstring fixes.

Not sure if I did this right :grimacing:

Agreed -- this is almost ready. Please add to the API docs (api.rst) and do the docstring fixes.

This looks good to me. Do we want to add this to the api page in the docs?

Rebased and ready for another round of reviews :wink:

The appveyor build failure looks unrelated to this change -- something about conda dependencies.

I am experiencing that in other projects. It is actually a bad download of miniconda, and powershell makes the error message unusable. Re-starting should fix it. (BTW miniconda is pre-installed on AppVeyor and that download is not needed.)

@ocefpaf We can squash on merge now in the web interface, so that makes things easier.

The appveyor build failure looks unrelated to this change -- something about conda dependencies.

@jhamman and @shoyer if the tests passes this is ready for another round of review. (Let me know if I should squash the previous ones to make it easier to review.)

@shoyer this is ready for another round of review.

Well, if you guys are mostly excited about using this for coordinate variables, another consistent choice would be to return a list of matching DataArrays. But if we want to return a Dataset, we should only do data variables, because it's weird to lose all the describing coordinates when you match, e.g., standard_name="air_temperature".

We agree with you and I prefer to return a Dataset.

Right now we always go to netCDF4-python to do this low-level CF interpretation stuff. If we start using xarray for that task we should improve xarray, to take advantage of the conventions (CF/SGRID/UGRID), instead of using xarray to just find the coords data. (For example: creating the z coordinates from non-dimension coordinates and add that to the Dataset coords automatically.)

It also seems consistent to me to return a Dataset.

Well, if you guys are mostly excited about using this for coordinate variables, another consistent choice would be to return a list of matching DataArrays. But if we want to return a Dataset, we should only do data variables, because it's weird to lose all the describing coordinates when you match, e.g., standard_name="air_temperature".

On Thu, May 5, 2016 at 7:05 AM, Kyle Wilcox notifications@github.com wrote:

@ocefpaf https://github.com/ocefpaf I agree that xarray model consistent should take precedence. I also use this method to (mostly) pull out coordinate variables and can continue to use netCDF4.Dataset to do that.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/pydata/xarray/pull/844#issuecomment-217163460

@ocefpaf I agree that keeping the xarray model consistent should take precedence. I also use this method to (mostly) pull out coordinate variables and can continue to use netCDF4.Dataset to do that.

After discussing with my CF guru (@rsignell-usgs) and the original author of the get_variables_by_attributes (@kwilcox) I jumped the fence and I am OK leaving the xarray model pure and filtering only the data variables.

should this filter both data variables and coordinates or only data variables?

I thought a little bit more about this and now I am on the fence. The pros of filtering only data variables are a nice and clean Dataset object, and overall consistency with the high level xarray model. The cons are that we cannot do the filtering on the coords (obviously), but most of the time that we need to do that we go to a lower level object like netCDF4.Dataset. However, it would be nice if both xarray and netCDF4 behaved the same way...

I am 51% with filtering both (and the current implementation does that) but I will leave the final decision to you.

An important design question: should this filter both data variables and coordinates or only data variables? My thought is that it's only worth filtering data variables -- filtering out unmatched coordinates is not very useful.

I understand that returning coordinates without a data variable associated to them seems weird to the high level model of xarray.Dataset , but I disagree that it is not very useful. In fact that is the most common operation I do: find coordinates based on axis, formula_terms, etc and construct common grids for plotting and/or building the z coords from non-dimension coords.

An important design question: should this filter both data variables and coordinates or only data variables? My thought is that it's only worth filtering data variables -- filtering out unmatched coordinates is not very useful.