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

DOC: Update of 'return_eigenvectors' description #9134

Merged
merged 10 commits into from
Aug 31, 2018
Merged

DOC: Update of 'return_eigenvectors' description #9134

merged 10 commits into from
Aug 31, 2018

Conversation

akahard2dj
Copy link
Contributor

Issued by @Gauthe. #9082.

@@ -1361,7 +1361,8 @@ def eigsh(A, k=6, M=None, sigma=None, which='LM', v0=None,
Returns
-------
w : array
Array of k eigenvalues
Array of k eigenvalues.

Copy link
Member

Choose a reason for hiding this comment

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

We don't need empty lines between the items in the same section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, It seems that there is a blank line in editing process. I will correct your point.


If `return_eigenvectors` is True, eigenvalues are sorted by algebraic value

If `return_eigenvectors` is False, eigenvalues are sorted by magnitude value
Copy link
Member

Choose a reason for hiding this comment

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

The term "absolute value" sounds more natural.

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 agree with you.
Generally speaking, 'absolution' is better that 'magnitude'.
Thanks for your quickly reviews, @ilayn.

Changes that reflect @ilayn reviews.
adding a more detailed which case description
For which = 'SM':
If `return_eigenvectors` is True, eigenvalues are sorted by algebraic value

If `return_eigenvectors` is False, eigenvalues are sorted by decreasing absolute value.

Choose a reason for hiding this comment

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

punctuation terminates this sentence, but not the ones on lines 1453, 1455, 1458, and 1461.

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 will revise the sentence with a punctuation termination.

@jeffyancey
Copy link

FWIW, the docstring for the entire method is rampant with inconsistent punctuation. I don't consider that important, but now is not a bad time to make the docstring consistent.

@ilayn ilayn added scipy.sparse.linalg Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org labels Aug 18, 2018
@akahard2dj
Copy link
Contributor Author

@jeffyancey I agree with you. I'll do more thorough checking and document work.

Working with a document consistent issued by @jeffyancey.
@akahard2dj
Copy link
Contributor Author

@jeffyancey I fixed docstring consistency for a punctuation terminate.
Could you review this commit?

Copy link

@jeffyancey jeffyancey left a comment

Choose a reason for hiding this comment

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

You'll have my approval as soon as which is marked as a variable with `'s

OPinv : N x N matrix, array, sparse matrix, or LinearOperator
See notes in sigma, above.
return_eigenvectors : bool
Return eigenvectors (True) in addition to eigenvalues
Return eigenvectors (True) in addition to eigenvalues. This value determines
the order in which eigenvalues are sorted. The sort order is also dependent on the which variable.

Choose a reason for hiding this comment

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

could you make which => which?

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 fixed your request.
Changed general word to marked as a variable

which is marked as a variable
@rgommers rgommers changed the title DOC: Update of 'return_eigencvectors' description DOC: Update of 'return_eigenvectors' description Aug 24, 2018

Parameters
----------
A : An N x N matrix, array, sparse matrix, or LinearOperator representing
the operation A * x, where A is a real symmetric matrix
For buckling mode (see below) A must additionally be positive-definite
the operation A * x, where A is a real symmetric matrix.
Copy link
Member

Choose a reason for hiding this comment

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

Asterisks have special meaning in ReST, so should wrap A*x into double backticks.

Copy link
Member

Choose a reason for hiding this comment

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

Also it's better to use A @ b for matrix multiplication.

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 fixed the parts that the two, @ev-br and @ilayn , asked .
And I also changed matrix multiplication operator '*' to '@' in the other part of eigsh.

Please review this commit.

- changes asterisks to matrix multiplication operator '@'
- wrap A @ x into double backticks
changes matrix multiplication operator '*' into '@'
@ogauthe
Copy link
Contributor

ogauthe commented Aug 27, 2018

Line 1374 should be
A @ x = w * M @ x.
since w is a scalar in the definition of the generalized eigenvalue problem.

@akahard2dj
Copy link
Contributor Author

@gauth I forgot that w was scala.
Thank for your valuable review.

@rgommers rgommers added this to the 1.2.0 milestone Aug 31, 2018
@rgommers rgommers merged commit 233e0e4 into scipy:master Aug 31, 2018
@rgommers
Copy link
Member

LGTM now, merging. Thanks @akahard2dj, all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.sparse.linalg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants