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

DOC: proposed fixes for issues #7622 and #7914 #8890

Merged
merged 1 commit into from
Apr 6, 2017
Merged

DOC: proposed fixes for issues #7622 and #7914 #8890

merged 1 commit into from
Apr 6, 2017

Conversation

sjpeterson
Copy link
Contributor

@sjpeterson sjpeterson commented Apr 4, 2017 •
edited by eric-wieser

(#7622 and #7914)

eric-wieser
eric-wieser reviewed Apr 4, 2017
View reviewed changes
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Commit message probably doesn't need the word "proposed" in it, because if we merge it it'll no longer be just a proposal ;)

numpy/core/numeric.py Outdated
@@ -2145,8 +2145,12 @@ def fromfunction(function, shape, **kwargs):
The function is called with N parameters, where N is the rank of
`shape`. Each parameter represents the coordinates of the array
varying along a specific axis. For example, if `shape`
were ``(2, 2)``, then the parameters in turn be (0, 0), (0, 1),
(1, 0), (1, 1).
were ``(2, 2)``, then the parameters would in turn be
Copy link
Member

Choose a reason for hiding this comment

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

"in turn" is now incorrect here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not an English major, but I believe this to be correct usage whereas the previous wording was a bit sketchy. "In turn" generally means "as a consequence of" when used infix, but "one after the other when used postfix.

https://en.oxforddictionaries.com/definition/in_turn

Copy link
Member

Choose a reason for hiding this comment

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

Well at the very least, the intended meaning of "in turn" before and after this change is different. While what you have now might be correct, I don't think "in turn" adds any clarity at all here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, I'll remove it.

numpy/core/numeric.py Outdated
[ 1, 1 ]])
and
array([[ 0, 1 ],
[ 0, 1 ]])
Copy link
Member

Choose a reason for hiding this comment

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

Does this render correctly in the web docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. It does not render quite correctly. I'll fix that. Do I close this pull request and create a new one or do I update this one somehow? I'm a novice contributor so forgive me if this should be obvious.

Copy link
Member

Choose a reason for hiding this comment

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

Don't close it. If you add new commits, just push them to the branch you made this PR from and they'll show up here automatically.

eric-wieser
eric-wieser reviewed Apr 4, 2017
View reviewed changes
numpy/add_newdocs.py Outdated
@@ -1453,7 +1453,7 @@ def luf(lamdaexpr, *args, **kwargs):
When True, yield `x`, otherwise yield `y`.
x, y : array_like, optional
Values from which to choose. `x` and `y` need to have the same
shape as `condition`.
shape as `condition`, or be broadcastable to that shape.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really correct either, although it's definitely closer to correct.

(3,1) and (1, 3) are not broadcastable to (), yet np.where(True, np.zeros((3, 1)), np.zeros((1, 3))) is valid.

"x, y, and condition must be broadcastable together" is the correct description, although there might be a better way to phrase that.

Note that until #8599 is fixed, the description you have here is correct for ma.where.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, how about something like

Unless the shape of condition is (), x and y need to have the same shape as condition or be broadcastable to that shape.

Copy link
Member

Choose a reason for hiding this comment

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

That's also wrong though, for x, y, condition with shapes (3, 1, 1), (1, 3, 1), (1, 1, 3). (3, 1, 1), (1, 3, 1) is not broadcastable to (1, 1, 3), but it is broadcastable with (1, 1, 3)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.. which would also cover the aforementioned (1,3), (3,1) and () case as well as the case where all shapes are the same, right?

eric-wieser
eric-wieser reviewed Apr 4, 2017
View reviewed changes
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

You should probably squash these commits too

numpy/core/numeric.py Outdated
were ``(2, 2)``, then the parameters in turn be (0, 0), (0, 1),
(1, 0), (1, 1).
were ``(2, 2)``, then the parameters would be
``array([[0, 0],[1, 1]])`` and ``array([[0, 1], [0, 1]])``
Copy link
Member

Choose a reason for hiding this comment

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

Missing space

numpy/ma/core.py Outdated
@@ -6959,7 +6959,7 @@ def where(condition, x=_NoValue, y=_NoValue):
element from `x`, otherwise from `y`.
x, y : array_like, optional
Values from which to choose. `x` and `y` need to have the same shape
as condition, or be broadcast-able to that shape.
as condition, or be broadcastable to that shape.
Copy link
Member

Choose a reason for hiding this comment

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

"have the same shape" is a subset of "be broadcastable to that shape", so arguably isn't needed

Copy link
Member

Choose a reason for hiding this comment

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

Actually #8599 is merged now anyway - so this should be updated to be the same as np.where

@sjpeterson
Copy link
Contributor Author

These errors can't really be due to something in the pull request, can they?

fatal: unable to access 'https://github.com/numpy/numpy.git/': Unknown SSL protocol error in connection to github.com:443

eric-wieser
eric-wieser approved these changes Apr 5, 2017
View reviewed changes
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Lgtm. Tests are unrelated, but I'll reopen the pr to retest to remove the failure.

In future, you can use [ci skip] in the message to skip tests for this kind of change

@eric-wieser eric-wieser closed this Apr 5, 2017
@eric-wieser eric-wieser reopened this Apr 5, 2017
@eric-wieser
Copy link
Member

Thanks @sjpet

@eric-wieser eric-wieser merged commit aa746c6 into numpy:master Apr 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgommers rgommers rgommers left review comments

@eric-wieser eric-wieser eric-wieser approved these changes

Assignees
No one assigned
Labels
04 - Documentation component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sjpeterson @eric-wieser @rgommers