8000 ENH: allow single input argument in numpy.broadcast by kohr-h · Pull Request #6905 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

ENH: allow single input argument in numpy.broadcast #6905

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
Merged
merged 3 commits into from
Jan 6, 2016
Merged

ENH: allow single input argument in numpy.broadcast #6905

merged 3 commits into from
Jan 6, 2016

Conversation

kohr-h
Copy link
Contributor
@kohr-h kohr-h commented Dec 30, 2015

Fixes #6899. The documentation does not mention any restriction, so I guess there is no need to adapt it?

This was referenced Dec 30, 2015
Closed
Merged
@jaimefrio
Copy link
Member

This change requires some tests to validate that it works.

Also, there are three functions:

  • PyArray_MultiIterFromObjects
  • PyArray_MultiIterNew
  • arraymultiter_new

doing almost the exact same thing, and this change brings them slightly apart, which is not a good thing.

Probably the right thing to do is to refactor the code so that there is a single implementation of the multi-iterator constructor that gets called by the others. But at the very least, the change should be made on all three.

@kohr-h
Copy link
Contributor Author
kohr-h commented Jan 3, 2016

This change requires some tests to validate that it works.

Where would such a test go? Would it be sufficient to just always use np.broadcast in this test, assuming there is also a single-array test case?

Also, there are three functions:

PyArray_MultiIterFromObjects
PyArray_MultiIterNew
arraymultiter_new

doing almost the exact same thing, and this change brings them slightly apart, which is not a good thing.

Probably the right thing to do is to refactor the code so that there is a single implementation of the multi-iterator constructor that gets called by the others. But at the very least, the change should be made on all three.

I can easily do the least and adapt those three functions. Tests are also relevant here, I guess.
Regarding the common code, I assume you are referring (roughly) to this portion of the constructor. Do you want me to make an attempt? How about names for small helpers then, any suggestion?

@kohr-h
Copy link
Contributor Author
kohr-h commented Jan 3, 2016

I just found the dedicated TestBroadcast case.

@kohr-h
Copy link
Contributor Author
kohr-h commented Jan 3, 2016

Single-arg broadcast has a test now. I did not find a nice way to modify the existing list and get the desired result, so I added a few extra lines.

@kohr-h
Copy link
Contributor Author
kohr-h commented Jan 3, 2016

I took the easy route and just changed numbers and error messages three times. The "proper" code refactoring goes beyond my knowledge of the C part.
broadcast does not seem to be used very much in the rest of the library, I just found one place in stride_tricks.py where a distinction had to be made between len(args) == 1 and all other cases. This check has been removed.

Question: are there dedicated tests for the PyArray_MultiIterFromObjects, PyArray_MultiIterNew, arraymultiter_new functions where a case for length 1 could be added?

jaimefrio
jaimefrio reviewed Jan 4, 2016
View reviewed changes
numpy/core/tests/test_numeric.py
@@ -2207,11 +2207,19 @@ def test_broadcast_in_args(self):
for a, ia in zip(arrs, mit.iters):
assert_(a is ia.base)

# gh-6899
Copy link
Member

Choose a reason for hiding this comment

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

I think this deserves its own test method, test_broacast_single_arg or something along that line.

@homu
Copy link
Contributor
homu commented Jan 4, 2016

☔ The latest upstream changes (presumably #6900) made this pull request unmergeable. Please resolve the merge conflicts.

@kohr-h kohr-h force-pushed the issue-6899__broadcast_with_one_arg branch from 7e6a365 to ec4d0b8 Compare January 4, 2016 10:04
@kohr-h kohr-h force-pushed the issue-6899__broadcast_with_one_arg branch from ec4d0b8 to 361c0d5 Compare January 4, 2016 10:47
@kohr-h
Copy link
Contributor Author
kohr-h commented Jan 4, 2016

Rebased and streamlined the commits. Any further action necessary?

@jaimefrio
8000
Copy link
Member

One last request: I think this is worthy of a mention in the release notes.

@kohr-h
Copy link
Contributor Author
kohr-h commented Jan 5, 2016

Sure, under which heading? Changes?

@kohr-h
Copy link
Contributor Author
kohr-h commented Jan 6, 2016

Looking ok?

jaimefrio added a commit that referenced this pull request Jan 6, 2016
@jaimefrio
Merge pull request #6905 from kohr-h/issue-6899__broadcast_with_one_arg
55b5972 
ENH: allow single input argument in numpy.broadcast
@jaimefrio jaimefrio merged commit 55b5972 into numpy:master Jan 6, 2016
@jaimefrio
Copy link
Member

Thanks Holger, in it goes.

@homu homu mentioned this pull request Jan 6, 2016
Merged
@kohr-h
Copy link
Contributor Author
kohr-h commented Jan 7, 2016

My pleasure!

@kohr-h kohr-h deleted the issue-6899__broadcast_with_one_arg branch January 7, 2016 00:00
eric-wieser added a commit to eric-wieser/numpy that referenced this pull request May 13, 2019
@eric-wieser
ENH: Allow broadcast to be called with zero arguments
a3a19da 
Follows on from numpygh-6905 which reduced the limit from 2 to 1. Let's go all the way to zero.

Just as for `broadcast(broadcast(a), b)` is interpreted as `broadcast(a, b)` , this change interprets
`broadcast(broadcast(), a)` as `broadcast(a)`.
@eric-wieser eric-wieser mentioned this pull request May 13, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
01 - Enhancement component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kohr-h @jaimefrio @homu @charris
0