8000 Added an 'elementary' function. by mttpgn · Pull Request #4573 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

Added an 'elementary' function. #4573

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
Closed
wants to merge 10 commits into from
Closed

Added an 'elementary' function. #4573

wants to merge 10 commits into from

Conversation

mttpgn
Copy link
@mttpgn mttpgn commented Mar 31, 2014

The new function creates an identity array that's had a single row operation performed on it. Quite useful when doing matrix multiplication. https://en.wikipedia.org/wiki/Elementary_matrix

perimosocordiae
perimosocordiae reviewed Apr 8, 2014
View reviewed changes
numpy/lib/twodim_base.py
----------
N : int
The size of the NxN array to be returned. Elementary matrices
are be square.
Copy link
Contributor

Choose a reason for hiding this comment

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

"are square by definition", perhaps?

@perimosocordiae
Copy link
Contributor

Im not sure whether this function should be added, but it will definitely need test cases as well if it is.

larsmans
larsmans reviewed Apr 9, 2014
View reviewed changes
numpy/lib/twodim_base.py

"""
m=eye(N, dtype=dtype)
if j==None and t==None:
Copy link
Contributor

Choose a reason for hiding this comment

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

PEP8 wants spaces around operators. Also checking for None should be done with is.

@mttpgn
Copy link
Author
mttpgn commented Apr 9, 2014

Tests are still forthcoming.

@njsmith
Copy link
Member
njsmith commented Apr 9, 2014

I haven't looked at this in detail, but yes, please don't use np.matrix in
new code or docs.

For now the best way to write matrix multiplication is just as a call to
np.dot.
On 9 Apr 2014 22:24, "Matt Pagan" notifications@github.com wrote:

Tests are still forthcoming.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4573#issuecomment-40018304
.

@mttpgn
Copy link
Author
mttpgn commented Jul 26, 2014

So, I've added test coverage, switched to using np.dot instead of np.matrix, dropped the string formatting, and reworded the documentation. What do numpy devs think of this patch? Any other thoughts?

@juliantaylor
Copy link
Contributor

hm elementary matrices may be useful for mathematical notation, but why would you want to use them computations?
E.g. if you want to swap two rows of a matrix, it is far more efficient to just swap them instead of creating an elementary matrix and the doing a very expensive matrix multiplication.

@larsmans
Copy link
Contributor

You might want to compose several linear transformations by multiplying their corresponding matrices together.

@juliantaylor
Copy link
Contributor

hm it would still be very inefficient. But its probably useful for code clarity on operations where performance does not matter and could become more useful for other cases when numpy gains a smart matrix-dot chaining function.

@njsmith
Copy link
Member
njsmith commented Jul 26, 2014

Other cases I can imagine it might be useful:

  • have an API that takes an arbitrary matrix, want to pass in an elementary
    matrix
  • teaching
    On 26 Jul 2014 13:51, "Julian Taylor" notifications@github.com wrote:

hm it would still be very inefficient. But its probably useful for code
clarity on operations where performance does not matter and could become
more useful for other cases when numpy gains a smart matrix-dot chaining
function.

—
Reply to this email directly or view it on GitHub
#4573 (comment).

@juliantaylor
Copy link
Contributor 
In [11]: elementary(3,0,1, multiplier=3)
Out[11]: 
array([[ 1.,  0.,  0.],
       [ 3.,  1.,  0.],
       [ 0.,  0.,  1.]])

is the last 1 on the diagonal correct here? it isn't a multiplier anymore in this case.

@charris
Copy link
Member
charris commented Aug 8, 2014

Needs rebase.

ewmoore
ewmoore reviewed Aug 12, 2014
View reviewed changes
numpy/lib/twodim_base.py
performed.
multiplier : scalar
If set, the factor by which a given row will be multiplied.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to document the dtype arg?

@argriffing
Copy link
Contributor

I was looking for a numpy elementary function that returns row j of the identity matrix without actually constructing the identity matrix, and I found this PR which does a different thing.

@eric-wieser
Copy link
Member

@argriffing: Perhaps np.unit(shape, ind) as a shorthand for arr = zeros(shape) with arr[ind] == 1, and sum(arr) == 1?

@mattip
Copy link
Member
mattip commented Feb 13, 2019

Is someone willing to champion this and shepherd it through the review process? If not perhaps we should close it.

@seberg
Copy link
Member
seberg commented Apr 26, 2019

@mttpgn this seems to still requires some work or decision. Unless you comment/wish to continue, I would probably:

  • close the PR
  • create a new issues to note that there is a start for fixing this in this PR. (Or modify the existing one)

in a few weeks. Of course the PR can be reactivated at any time. (tag:PR-possibly-close)

Otherwise, it probably mostly needs the decision that we actually want this in, including a ping to the mailing list.

@mttpgn mttpgn closed this Apr 30, 2019
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 55 - Needs work component: numpy.lib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
0