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

Improve docstring for optax.power_iteration. #1215

Merged

Conversation

carlosgmartin
Copy link
Contributor

Minor improvements to the docstring for optax.power_iteration, including explaining the connection to the spectral radius of the matrix and noting that a dominant eigenvalue might not exist.

Copy link
Collaborator

@vroulet vroulet left a comment

Choose a reason for hiding this comment

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

Thanks!

@vroulet
Copy link
Collaborator

vroulet commented Mar 4, 2025

Sorry thinking a bit more about it, the wikipedia reference should be enough for additional mathematical details. I'm not sure we need to lengthen the docs here.

@carlosgmartin
Copy link
Contributor Author

carlosgmartin commented Mar 4, 2025

The reason I added this is because the current doc makes it seem like a ("the") dominant eigenvalue exists. But it might not, and in that case, the algorithm fails to converge. Also, one of the common uses of this function is computing the spectral radius, but the function might not always work for that end, for the aforementioned reason.

That's the intent behind my clarification.

@vroulet
Copy link
Collaborator

vroulet commented Mar 6, 2025

@carlosgmartin, thanks for the clarification.
Would the following note be good with you (I'll take care of the merge internally)?

  .. note::
    If the matrix is not diagonalizable or the dominant eigenvalue is not unique, the algorithm may not converge. 

and add at the beginning of the docstring

dominant eigenvalue (i.e, the spectral radius)

@carlosgmartin
Copy link
Contributor Author

Sure.

@carlosgmartin
Copy link
Contributor Author

@vroulet To clarify, should I do anything, or do you take it from here?

@vroulet
Copy link
Collaborator

vroulet commented Mar 11, 2025

I took care of it, it should get merged soon. Thanks again

@copybara-service copybara-service bot merged commit 84e88e2 into google-deepmind:main Mar 11, 2025
12 checks passed
@carlosgmartin carlosgmartin deleted the power_iteration_docstring branch March 12, 2025 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants