Skip to content

Remove deprecated linear algebra functions (and methods) #22841

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

Remove deprecated linear algebra functions (and methods) #22841

vishwakftw wants to merge 6 commits into from

Conversation

@vishwakftw
Copy link
Contributor

@vishwakftw vishwakftw commented Jul 13, 2019

Changelog:

  • Removed the following linear algebra functions in PyTorch in favor of the renamed operations

    • btrifact (use lu instead)
    • btrifact_with_info (use lu with get_infos=True instead)
    • btrisolve (use lu_solve instead)
    • btriunpack (use lu_unpack instead)
    • gesv (use solve instead)
    • pstrf (use cholesky instead)
    • potrf (use cholesky instead)
    • potri (use cholesky_inverse instead)
    • potrs (use cholesky_solve instead)
    • trtrs (use triangular_solve instead)

  • Removed dead code after the removal of pstrf

Test Plan:

  • All existing tests should pass to verify that the removal is clean

Closes #22832

@vishwakftw
Remove deprecated linear algebra functions (and methods)
a59d805 
Changelog:
- Removed the following linear algebra functions in PyTorch in favor of the renamed operations
  - `btrifact` (use `lu` instead)
  - `btrifact_with_info` (use `lu` with `get_infos=True` instead)
  - `btrisolve` (use `lu_solve` instead)
  - `btriunpack` (use `lu_unpack` instead)
  - `gesv` (use `solve` instead)
  - `pstrf` (use `cholesky` instead)
  - `potrf` (use `cholesky` instead)
  - `potri` (use `cholesky_inverse` instead)
  - `potrs` (use `cholesky_solve` instead)
  - `trtrs` (use `triangular_solve` instead)

- Removed dead code after the removal of `pstrf`

Test Plan:
- All existing tests should pass to verify that the removal is clean
@pytorchbot pytorchbot added module: cpu CPU specific problem (e.g., perf, algorithm) module: docs Related to our documentation, both in docs/ and docblocks module: operators labels Jul 13, 2019
@vishwakftw vishwakftw added the module: bc-breaking Related to a BC-breaking change label Jul 13, 2019
@pytorchbot pytorchbot added the module: internals Related to internal abstractions in c10 and ATen label Jul 13, 2019
@xuhdev xuhdev mentioned this pull request Jul 14, 2019
Closed
@zou3519 zou3519 self-requested a review July 16, 2019 16:17
@zou3519
Copy link
Contributor

zou3519 commented Jul 16, 2019

I'll take a look by EOD tomorrow (Wednesday) but if anyone gets to this earlier, please feel free to review

@zou3519 zou3519 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jul 16, 2019
zou3519
zou3519 reviewed Jul 16, 2019
View reviewed changes
torch/functional.py
return torch._C._VariableFunctions.chain_matmul(matrices)


def pstrf(a, upper=True, out=None):
Copy link
Contributor

@zou3519 zou3519 Jul 16, 2019

Choose a reason for hiding this comment

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

If we wanted to be even safer we could just make this a hard error and fully remove it in the next release. How long have these functions been deprecated for already?

@zou3519
Copy link
Contributor

zou3519 commented Jul 16, 2019

@pytorchbot rebase this please

@pytorchbot
Copy link
Collaborator

pytorchbot commented Jul 16, 2019

Sorry, I can't merge this because there are conflicts. To merge this yourself, run the commands below:

git fetch origin master
git fetch [email protected]:vishwakftw/pytorch.git remove-deprecated-linalg
git checkout FETCH_HEAD
git merge origin/master
git push [email protected]:vishwakftw/pytorch.git HEAD:remove-deprecated-linalg

(To learn more about this bot, see Bot commands.)

zou3519
zou3519 reviewed Jul 16, 2019
View reviewed changes
aten/src/ATen/native/native_functions.yaml
CPU: legacy::cpu::_th_potri
CUDA: legacy::cuda::_th_potri

- func: pstrf(Tensor self, bool upper=True, Scalar tol=-1, *, Tensor(a!) u, Tensor(b!) pivot) -> (Tensor(a!) u, Tensor(b!) pivot)
Copy link
Contributor

@zou3519 zou3519 Jul 16, 2019

Choose a reason for hiding this comment

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

I'm worried about C++ API users. I'm not sure if this function is exposed to the c++ API (I hope it's not) but we didn't add a deprecation warning there.

Copy link
Contributor

@zou3519 zou3519 Jul 16, 2019

Choose a reason for hiding this comment

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

@yf225 what are our guarantees on the C++ API? I think we just renamed things like gesv -> solve at some point in the past, but maybe the C++ API was more unstable then?

Copy link
Contributor

@yf225 yf225 Jul 16, 2019

Choose a reason for hiding this comment

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

I think tensor.pstrf is exposed to the C++ API because things inTensor.h are public APIs.

In general I think it's more important to maintain parity between Python and C++ frontend, and it's okay to break BC for C++ API here. (Here https://pytorch.org/cppdocs/ we also mentioned that "the C++ API should be considered “beta” stability" and "we may make major breaking changes to the backend in order to improve the API".)

zou3519
zou3519 approved these changes Jul 17, 2019
View reviewed changes
Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

Thank you! This needs a rebase I think.

@vishwakftw
Copy link
Contributor Author

vishwakftw commented Jul 17, 2019

Thank you for the reviews @zou3519 and @yf225.

I have rebased the branch and pushed it.

facebook-github-bot
facebook-github-bot reviewed Jul 17, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zdevito pushed a commit to zdevito/ATen that referenced this pull request Jul 19, 2019
@vishwakftw @facebook-github-bot
Remove deprecated linear algebra functions (and methods) (#22841)
a04c814 
Summary:
Changelog:
- Removed the following linear algebra functions in PyTorch in favor of the renamed operations
  - `btrifact` (use `lu` instead)
  - `btrifact_with_info` (use `lu` with `get_infos=True` instead)
  - `btrisolve` (use `lu_solve` instead)
  - `btriunpack` (use `lu_unpack` instead)
  - `gesv` (use `solve` instead)
  - `pstrf` (use `cholesky` instead)
  - `potrf` (use `cholesky` instead)
  - `potri` (use `cholesky_inverse` instead)
  - `potrs` (use `cholesky_solve` instead)
  - `trtrs` (use `triangular_solve` instead)

- Removed dead code after the removal of `pstrf`
Pull Request resolved: pytorch/pytorch#22841

Test Plan:
- All existing tests should pass to verify that the removal is clean

Closes pytorch/pytorch#22832

Differential Revision: D16346184

Pulled By: zou3519

fbshipit-source-id: f748d16ed7609c028de6adcbc28684d5a1af0678
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 19, 2019

@zou3519 merged this pull request in 6dfecc7.

@vishwakftw vishwakftw deleted the remove-deprecated-linalg branch July 23, 2019 12:05
@fdraxler fdraxler mentioned this pull request Jan 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yf225 yf225 yf225 left review comments

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@zou3519 zou3519 zou3519 approved these changes

Assignees

No one assigned

Labels

Merged module: bc-breaking Related to a BC-breaking change module: cpu CPU specific problem (e.g., perf, algorithm) module: docs Related to our documentation, both in docs/ and docblocks module: internals Related to internal abstractions in c10 and ATen open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove deprecated linear algebra wrappers

7 participants

@vishwakftw @zou3519 @pytorchbot @facebook-github-bot @yf225 @ezyang @mruberry