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

Merge broadcast_like docstrings, analyze implementation problem #3130

Merged

Conversation

DavidMertz
Copy link
Contributor

@DavidMertz DavidMertz commented Jul 14, 2019

This PR fixed the forked code connected to #3129.

@pep8speaks
Copy link

pep8speaks commented Jul 14, 2019

Hello @DavidMertz! 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-15 14:37:22 UTC

@codecov
Copy link

codecov bot commented Jul 15, 2019

Codecov Report

Merging #3130 into master will decrease coverage by 0.28%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3130      +/-   ##
==========================================
- Coverage   95.99%   95.71%   -0.29%     
==========================================
  Files          63       63              
  Lines       12799    12799              
==========================================
- Hits        12287    12250      -37     
- Misses        512      549      +37

@dcherian
Copy link
Contributor

dcherian commented Jul 15, 2019

Thanks @DavidMertz

Line 2042 of dataset.py should be
return _broadcast_helper(args[1], exclude, dims_map, common_coords) i.e. args[1] instead of self.

Can you make that change (and the same change in dataarray.py, Line 1019)?

EDIT: I made the dataarray change directly.

@DavidMertz DavidMertz changed the title [WIP] Merge broadcast_like docstrings, analyze implementation problem Merge broadcast_like docstrings, analyze implementation problem Jul 15, 2019
@DavidMertz
Copy link
Contributor Author

DavidMertz commented Jul 15, 2019

Made changes suggested by @dcherian. This resolves the detected bug in the test. If CI passes, this should be ready to merge. Removed the temporary method .broadcast_like_naive() and the test using it.

@dcherian
Copy link
Contributor

Thanks for catching that @DavidMertz. Can you add back the original, simpler test too? This one is really just testing align. You could use @pytest.mark.parametrize to provide two different sets of arr1, arr2 arguments.

Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

LGTM.

@shoyer shoyer merged commit 8da3f67 into pydata:master Jul 16, 2019
@shoyer
Copy link
Member

shoyer commented Jul 16, 2019

thanks @DavidMertz and @dcherian !

dcherian added a commit to yohai/xarray that referenced this pull request Aug 3, 2019
* 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New .broadcast_like() incorrect
4 participants