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

Add broadcast_like. #3086

Merged
merged 6 commits into from
Jul 14, 2019
Merged

Add broadcast_like. #3086

merged 6 commits into from
Jul 14, 2019

Conversation

dcherian
Copy link
Contributor

@dcherian dcherian commented Jul 6, 2019

@pep8speaks
Copy link

pep8speaks commented Jul 6, 2019 •
edited
Loading

Hello @dcherian! 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-07-12 14:57:07 UTC

@shoyer
Copy link
Member

shoyer commented Jul 7, 2019

In principle, it feels like it would be better to have both xarray.broadcast() and broadcast_like written in terms of a lower level helper method that broadcasts only one object. This is what we do for align/reindex_like, for example. But in practice this is certainly way easier to write and probably good enough...

@codecov
Copy link

codecov bot commented Jul 8, 2019 •
edited
Loading

Codecov Report

Merging #3086 into master will decrease coverage by 0.01%.
The diff coverage is 98%.

@@            Coverage Diff             @@
##           master    #3086      +/-   ##
==========================================
- Coverage   95.99%   95.98%   -0.02%     
==========================================
  Files          63       63              
  Lines       12792    12809      +17     
==========================================
+ Hits        12280    12295      +15     
- Misses        512      514       +2

max-sixty
max-sixty reviewed Jul 8, 2019
View reviewed changes
xarray/core/alignment.py
data = _set_dims(array.variable)
coords = OrderedDict(array.coords)
coords.update(common_coords)
return DataArray(data, coords, data.dims, name=array.name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could potentially use ._replace, and then only supply the changed items (but not a big deal)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean

Copy link
Collaborator

Choose a reason for hiding this comment

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

i.e. return array._replace(data, coords)
So array keeps its type if someone inherited from DataArray and we're not coupled to specific DataArray properties if those ever change

Copy link
Member

Choose a reason for hiding this comment

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

I agree, but let's save this for a later refactor.

@max-sixty max-sixty mentioned this pull request Jul 8, 2019
Closed
@shoyer shoyer mentioned this pull request Jul 14, 2019
Closed
3 tasks
@shoyer
Copy link
Member

shoyer commented Jul 14, 2019

I'm merging this now, so @DavidMertz can put his doc/test improvements on top of it.

@shoyer shoyer merged commit 9438390 into pydata:master Jul 14, 2019
@shoyer
Copy link
Member

shoyer commented Jul 14, 2019

thanks @dcherian !

@dcherian dcherian deleted the broadcast_like branch July 14, 2019 21:14
@dcherian
Copy link
Contributor Author

Sounds good. @DavidMertz's docs/tests are much better!

@DavidMertz DavidMertz mentioned this pull request Jul 14, 2019
Closed
dcherian added a commit to yohai/xarray that referenced this pull request Aug 3, 2019
@dcherian
Merge branch 'master' into yohai-ds_scatter
318abaa 
* master: (68 commits)
  enable sphinx.ext.napoleon (pydata#3180)
  remove type annotations from autodoc method signatures (pydata#3179)
  Fix regression: IndexVariable.copy(deep=True) casts dtype=U to object (pydata#3095)
  Fix distributed.Client.compute applied to DataArray (pydata#3173)
  More annotations in Dataset (pydata#3112)
  Hotfix for case of combining identical non-monotonic coords (pydata#3151)
  changed url for rasterio network test (pydata#3162)
  to_zarr(append_dim='dim0') doesn't need mode='a' (pydata#3123)
  BUG: fix+test groupby on empty DataArray raises StopIteration (pydata#3156)
  Temporarily remove pynio from py36 CI build (pydata#3157)
  missing 'about' field (pydata#3146)
  Fix h5py version printing (pydata#3145)
  Remove the matplotlib=3.0 constraint from py36.yml (pydata#3143)
  disable codecov comments (pydata#3140)
  Merge broadcast_like docstrings, analyze implementation problem (pydata#3130)
  Update whats-new for pydata#3125 and pydata#2334 (pydata#3135)
  Fix tests on big-endian systems (pydata#3125)
  XFAIL tests failing on ARM (pydata#2334)
  Add broadcast_like. (pydata#3086)
  Better docs and errors about expand_dims() view (pydata#3114)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@max-sixty max-sixty max-sixty left review comments

@shoyer shoyer shoyer left review comments

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

Successfully merging this pull request may close these issues.

Add broadcast_like?
4 participants
@dcherian @pep8speaks @shoyer @max-sixty