Skip to content

Linear algebra/power iteration #2190

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

Merged
merged 10 commits into from
Jul 26, 2020

Conversation

zakademic
Copy link
Contributor

@zakademic zakademic commented Jul 9, 2020

Describe your change:

Added power iteration algorithm. Given a symmetric matrix and a random vector of same space, will find largest eigenvalue and corresponding eigenvector. Rayleigh quotient needed to compute eigenvalue given an eigenvector.

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

@TravisBuddy
Copy link

Hey @zakademic,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: a88eb620-c19e-11ea-8e31-7fd6e6dac888

Copy link
Member

@ruppysuppy ruppysuppy left a comment

Choose a reason for hiding this comment

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

Go through the CONTRIBUTING.md and make sure it passes the flake8 style standard

Copy link
Member

@ruppysuppy ruppysuppy left a comment

Choose a reason for hiding this comment

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

Add doctests

@cclauss cclauss added tests are failing Do not merge until tests pass require tests Tests [doctest/unittest/pytest] are required labels Jul 9, 2020
@cclauss
Copy link
Member

cclauss commented Jul 9, 2020

@cclauss
Copy link
Member

cclauss commented Jul 9, 2020

@QuantumNovice Your review please.

@zakademic
Copy link
Contributor Author

Is this a duplicate of
https://github.com/TheAlgorithms/Python/blob/master/linear_algebra/src/rayleigh_quotient.py

Oops I missed that when writing power iteration. The rayleigh quotient I wrote within it is a duplicate; I will remove. But power iteration is not a duplicate. Thanks!

@zakademic
Copy link
Contributor Author

Go through the CONTRIBUTING.md and make sure it passes the flake8 style standard

OK will do.

Comment on lines 3 to 20
def rayleigh_quotient(input_matrix: np.array, vector: np.array) -> float:

"""
Rayleigh Quotient.
Fine the Rayliegh quotient of matrix and vector. Given an eigenvector,
the Rayleigh quotient is the corresponding eigenvalue.

Input
input_matrix: input matrix.
Numpy array. np.shape(input_matrix) == (N,N).
vector: input vector.
Numpy array. np.shape(vector) == (N,) or (N,1)
"""

num = np.dot(vector.T, np.dot(input_matrix, vector))
den = np.dot(vector.T,vector)

return num/den
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove these lines and use this instead?

from rayleigh_quotient import rayleigh_quotient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on this now, thank you!

@zakademic
Copy link
Contributor Author

@QuantumNovice I actually removed Rayleigh quotient since in the power iteration algorithm the vectors are already normalized. This removes a dot product and division operation.

I had not written doctests before so I referenced what was happening in Rayleigh Quotient and tried to emulate that.

I am using VS Code now and set black as my auto formatter. I ran flake8 as well.

Thanks all for your help!

Changed convergence check line.

Co-authored-by: Christian Clauss <[email protected]>
Copy link
Member

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Nice!

Numpy array. np.shape(largest_eigenvector) == (N,) or (N,1).

>>> import numpy as np
>>> A = np.array([
Copy link
Member

Choose a reason for hiding this comment

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

Please use the same variable names as the function parameters so the reader does not get lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @cclauss this is a great point; I made the change :)

Comment on lines 81 to 83
eigs, eigvs = np.linalg.eigh(A)
eig_max = eigs[-1]
eigv_max = eigvs[:, -1]
Copy link
Member

Choose a reason for hiding this comment

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

Longer variable names would improve readability for learners.

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 made changes to be more clear and added some comments as well. Appreciate it!

Named tests more clearly.

Co-authored-by: Christian Clauss <[email protected]>
@TravisBuddy
Copy link

Hey @zakademic,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 1f69e7e0-cf02-11ea-9502-ed86ad10d0d8

@cclauss cclauss merged commit 977dfaa into TheAlgorithms:master Jul 26, 2020
@cclauss
Copy link
Member

cclauss commented Jul 26, 2020

Nice!!

stokhos pushed a commit to stokhos/Python that referenced this pull request Jan 3, 2021
* Initial commit of power iteration.

* Added more documentation for power iteration and rayleigh quotient

* Type hinting for rayleigh quotient

* Changes after running black and flake8.

* Added doctests, added unit tests. Removed Rayleigh quotient as it is not needed.

* Update linear_algebra/src/power_iteration.py

Changed convergence check line.

Co-authored-by: Christian Clauss <[email protected]>

* Update linear_algebra/src/power_iteration.py

Named tests more clearly.

Co-authored-by: Christian Clauss <[email protected]>

* Changed naming in test function to be more clear. Changed naming in doctests to match function call.

* Self running tests

Co-authored-by: Zeyad Zaky <[email protected]>
Co-authored-by: Christian Clauss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
require tests Tests [doctest/unittest/pytest] are required tests are failing Do not merge until tests pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants