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

Fix name loss when masking #2749

Merged
merged 5 commits into from
Feb 11, 2019
Merged

Fix name loss when masking #2749

merged 5 commits into from
Feb 11, 2019

Conversation

yohai
Copy link
Contributor

@yohai yohai commented Feb 7, 2019

I took a simple-minded strategy to fix: just mask and then rename the result to self.name. This is a slight overkill since the renaming will be done on every masking, not just on the edge cases when it's needed, but this is really a very small computational cost and it's much easier and bulletproof.

@pep8speaks
Copy link

pep8speaks commented Feb 7, 2019

Hello @yohai! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 11, 2019 at 14:16 Hours UTC

@shoyer
Copy link
Member

shoyer commented Feb 7, 2019

This looks fine, though I'd rather fix this at a lower level, if possible.

This line in computation.py sets the name on result DataArray objects:

name = result_name(args)

If keep_attrs=True (you'll need to thread the argument into apply_dataarray_ufunc from apply_ufunc), then we should just copy the name from the first argument instead, e.g., name = args[0].name if keep_attrs else result_name(args).

@yohai
Copy link
Contributor Author

yohai commented Feb 7, 2019

I agree that my solution is a bit hacky, but from a computational cost viewpoint they are identical (or almost. I don't think there's a way to avoid one if statement per call of where). I thought my solution is good because catching this at a high level captures all possible edge cases that we might miss at lower ones. But I can try to do what you suggested to.

@yohai
Copy link
Contributor Author

yohai commented Feb 11, 2019

@shoyer your fix works. I think it's ready to merge.

@shoyer shoyer merged commit 07cfc5a into pydata:master Feb 11, 2019
@shoyer
Copy link
Member

shoyer commented Feb 11, 2019

thanks Yohai!

@yohai yohai deleted the where_name branch February 12, 2019 01:46
dcherian pushed a commit to yohai/xarray that referenced this pull request Feb 14, 2019
* master:
  typo in whats_new (pydata#2763)
  Update computation.py to use Python 3 function signatures (pydata#2756)
  add h5netcdf+dask tests (pydata#2737)
  Fix name loss when masking (pydata#2749)
  fix datetime_to_numeric and Variable._to_numeric (pydata#2668)
  Fix mypy errors (pydata#2753)
  enable internal plotting with cftime datetime (pydata#2665)
  remove references to cyordereddict (pydata#2750)
  BUG: Pass kwargs to the FileManager for pynio engine (pydata#2380) (pydata#2732)
  reintroduce pynio/rasterio/iris to py36 test env (pydata#2738)
  Fix CRS being WKT instead of PROJ.4 (pydata#2715)
  Refactor (part of) dataset.py to use explicit indexes (pydata#2696)
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.

name of array disappear when apply "where" filter with Datarray but not with np.array
3 participants