Rename potrf to cholesky #12699
Rename potrf to cholesky #12699
Conversation
This PR performs a renaming of the function `potrf` responsible for the Cholesky decomposition on positive definite matrices to `cholesky` as NumPy and TF do. Billing of changes - make potrf cname in Declarations.cwrap - modify the function names in ATen/core - modify the function names in Python frontend - issue warnings when potrf is called to notify users of the change
|
Please don't call it cholesky unless the default is lower triangular. I might add that I would have preferred if we could have had this coordinated with the batch cholesky. |
|
I do concur with you regarding the lower triangular property. I'll see what can be done - but on a side note: won't the docs help with this? Regarding batch_potrf, there seems to be a lot in that PR (potrf, tril and triu), and thought it would take a while before being merged into master. The changes in this PR will perhaps affect only the naming for batch_potrf and maybe doc strings and tests, which is why I isolated this separately. |
|
@t-vi @vishwakftw Would you two mind coming up with a plan for how to order these diffs? |
|
The batch potrf needs @vishwakftw 's batch inverse #9949 to go in first and a rebase is waiting on that. Richard left a bunch of review comments on the bug, so if Vishwak is fast to fix them, I'd redo the potrf patch. I don't think tril will be a huge delay. |
|
"So my own patch first" is what everyone finds best. 🤷♂️ But yeah, I can live with any ordering, as long as it makes everyone else happy. |
|
@t-vi The ordering is something I don't mind either. It's just that there will be a lot of additional cleaning up to do after your batch potrf PR lands. But it would be equally hard for you if this PR lands, so I think it needs more discussion. I opened this PR after having a chat with @zou3519 (hence the cc). But yes, maybe renaming potrf to cholesky could be part of a larger move on making PyTorch more NumPy-like :) , and this PR could be deferred accordingly. |
|
No, no, you should go first. Does this PR even conflict with the batch_inverse? If not it wouldn't even have to wait. |
|
This PR doesn't conflict with batch inverse. It only conflicts with batch potrf. |
|
@zou3519 Is this good to go? |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Unfortunately we're going to need to fix some internal stuff to track this name change, so we can't land this quite yet (your job is done though!). @zou3519 are you going to handle it? |
|
@ezyang yes, can do :) |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
I am resolving conflicts, will update the branch shortly. |
|
On my way to importing it and merging it... |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: This PR performs a renaming of the function `potrf` responsible for the Cholesky decomposition on positive definite matrices to `cholesky` as NumPy and TF do. Billing of changes - make potrf cname for cholesky in Declarations.cwrap - modify the function names in ATen/core - modify the function names in Python frontend - issue warnings when potrf is called to notify users of the change Reviewed By: soumith Differential Revision: D10528361 Pulled By: zou3519 fbshipit-source-id: 19d9bcf8ffb38def698ae5acf30743884dda0d88
|
Oh, this got merged in. Thank you @zou3519 . |
5 participants
This PR performs a renaming of the function
potrfresponsible for the Choleskydecomposition on positive definite matrices to
choleskyas NumPy and TF do.Billing of changes
cc: @zou3519