Skip to content

[Ready] Make potrs batched#13453

Closed
vishwakftw wants to merge 7 commits intopytorch:masterfrom
vishwakftw:batch_potrs
Closed

[Ready] Make potrs batched#13453
vishwakftw wants to merge 7 commits intopytorch:masterfrom
vishwakftw:batch_potrs

Conversation

@vishwakftw
Copy link
Copy Markdown
Contributor

@vishwakftw vishwakftw commented Nov 1, 2018 •
edited
Loading

  • This is a straightforward PR, building up on the batch inverse PR, except for one change:
    • The GENERATE_LINALG_HELPER_n_ARGS macro has been removed, since it is not very general and the resulting code is actually not very copy-pasty.

Billing of changes:

  • Add batching for potrs
  • Add relevant tests
  • Modify doc string

Minor changes:

  • Remove _gesv_single, _getri_single from aten_interned_strings.h.
  • Add test for CUDA potrs (2D Tensor op)
  • Move the batched shape checking to LinearAlgebraUtils.h

vishwakftw
vishwakftw commented Nov 2, 2018
View reviewed changes
Comment thread test/test_cuda.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@vishwakftw
Make potrs batched
f9f6d69 
- This is straightforward PR, building up on the batch inverse PR, except for one change:
  - The GENERATE_LINALG_HELPER_n_ARGS macro has been removed, since it is not very general
    and the resulting code is actually not very copy-pasty.
@vishwakftw
Add tests for batched potrs and modify doc strings for batched behaviour
3fe5df3
@vishwakftw
Complete tests for potrs
551360a
@vishwakftw vishwakftw changed the title [WIP] Make potrs batched [Ready] Make potrs batched Nov 3, 2018
@vishwakftw
Copy link
Copy Markdown
Contributor Author

vishwakftw commented Nov 3, 2018

@zou3519 This is ready for review, just for your information.

@zou3519
Copy link
Copy Markdown
Contributor

zou3519 commented Nov 5, 2018

@vishwakftw I'll take a look later today or tomorrow

@zou3519 zou3519 self-assigned this Nov 5, 2018
@vishwakftw
Copy link
Copy Markdown
Contributor Author

vishwakftw commented Nov 6, 2018

Thank you, appreciate it. :-)

@zou3519
Copy link
Copy Markdown
Contributor

zou3519 commented Nov 6, 2018

No, thank you for your contribution :)

@zou3519 zou3519 self-requested a review November 6, 2018 16:29
@zou3519 zou3519 removed their assignment Nov 6, 2018
zou3519
zou3519 reviewed Nov 6, 2018
View reviewed changes
Comment thread aten/src/ATen/Declarations.cwrap Outdated
zou3519
zou3519 reviewed Nov 6, 2018
View reviewed changes
Comment thread aten/src/ATen/native/BatchLinearAlgebra.cpp Outdated

template<class scalar_t>
void lapackGetri(int n, scalar_t *a, int lda, int *ipiv, scalar_t *work, int lwork, int *info) {
void lapackGetri(int n, scalar_t* a, int lda, int* ipiv, scalar_t* work, int lwork, int* info) {

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

facebook-github-bot
facebook-github-bot reviewed Nov 6, 2018
View reviewed changes
Copy link
Copy Markdown
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.

zou3519
zou3519 reviewed Nov 6, 2018
View reviewed changes
Comment thread aten/src/ATen/native/cuda/BatchLinearAlgebra.cu Outdated
zou3519
zou3519 reviewed Nov 6, 2018
View reviewed changes
Comment thread aten/src/ATen/native/cuda/BatchLinearAlgebra.cu Outdated
zou3519
zou3519 reviewed Nov 6, 2018
View reviewed changes
Comment thread tools/autograd/gen_python_functions.py
'_cumsum.*', '_cumprod.*', '_sum.*', '_prod.*', '_th_.*',
'arange.*', 'range.*', '_gesv.*', '_getri.*', '_inverse.*', 'slice',
'randint(_out)?',
'arange.*', 'range.*', '_gesv.*', '_getri.*', '_inverse.*', '_potrs.*',

This comment was marked as off-topic.

Sign in to view

zou3519
zou3519 reviewed Nov 6, 2018
View reviewed changes
Comment thread torch/_torch_docs.py
zou3519
zou3519 reviewed Nov 6, 2018
View reviewed changes
Comment thread tools/autograd/derivatives.yaml
zou3519
zou3519 reviewed Nov 6, 2018
View reviewed changes
Comment thread test/common_utils.py Outdated
zou3519
zou3519 reviewed Nov 6, 2018
View reviewed changes
Comment thread test/test_torch.py Outdated
b = cast(torch.randn(2, 1, 3, 4, 6))
L = get_cholesky(A, upper)
x = torch.potrs(b, L, upper=upper)
x_exp = torch.Tensor(solve(A.cpu().numpy(), b.cpu().numpy()))

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

zou3519
zou3519 reviewed Nov 6, 2018
View reviewed changes
Comment thread test/test_torch.py Outdated
self.assertEqual(x.data, cast(x_exp))

# broadcasting A
A = cast(random_symmetric_pd_matrix(4))

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

zou3519
zou3519 reviewed Nov 6, 2018
View reviewed changes
Comment thread test/test_torch.py Outdated
@vishwakftw
CR.2
fc612f6 
- minor refactor for potrs tests
zou3519
zou3519 approved these changes Nov 7, 2018
View reviewed changes
Copy link
Copy Markdown
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.

lgtm, thank you @vishwakftw!

facebook-github-bot
facebook-github-bot reviewed Nov 7, 2018
View reviewed changes
Copy link
Copy Markdown
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.

@vishwakftw
Copy link
Copy Markdown
Contributor Author

vishwakftw commented Nov 9, 2018

@zou3519 is there anything that I need to do?

facebook-github-bot
facebook-github-bot reviewed Nov 9, 2018
View reviewed changes
Copy link
Copy Markdown
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.

@zou3519
Copy link
Copy Markdown
Contributor

zou3519 commented Nov 9, 2018

@vishwakftw I think it should be good, I'll let you know if any action is required

@vishwakftw
Copy link
Copy Markdown
Contributor Author

vishwakftw commented Nov 9, 2018

@zou3519 just a notification: there were merge conflicts after the recent changes to Tensor.h/TensorMethods.h/Type.h. I fixed them.

facebook-github-bot
facebook-github-bot reviewed Nov 9, 2018
View reviewed changes
Copy link
Copy Markdown
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 Nov 9, 2018
@vishwakftw @facebook-github-bot
Make potrs batched (#13453)
f3a2298 
Summary:
- This is a straightforward PR, building up on the batch inverse PR, except for one change:
  - The GENERATE_LINALG_HELPER_n_ARGS macro has been removed, since it is not very general and the resulting code is actually not very copy-pasty.

Billing of changes:
- Add batching for `potrs`
- Add relevant tests
- Modify doc string

Minor changes:
- Remove `_gesv_single`, `_getri_single` from `aten_interned_strings.h`.
- Add test for CUDA `potrs` (2D Tensor op)
- Move the batched shape checking to `LinearAlgebraUtils.h`
Pull Request resolved: pytorch/pytorch#13453

Reviewed By: soumith

Differential Revision: D12942039

Pulled By: zou3519

fbshipit-source-id: 1b8007f00218e61593fc415865b51c1dac0b6a35
@vishwakftw vishwakftw deleted the batch_potrs branch November 10, 2018 03:42
@gpleiss gpleiss mentioned this pull request May 28, 2019
Closed
14 tasks
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@vishwakftw @facebook-github-bot
Make potrs batched (pytorch#13453)
4edfe0b 
Summary:
- This is a straightforward PR, building up on the batch inverse PR, except for one change:
  - The GENERATE_LINALG_HELPER_n_ARGS macro has been removed, since it is not very general and the resulting code is actually not very copy-pasty.

Billing of changes:
- Add batching for `potrs`
- Add relevant tests
- Modify doc string

Minor changes:
- Remove `_gesv_single`, `_getri_single` from `aten_interned_strings.h`.
- Add test for CUDA `potrs` (2D Tensor op)
- Move the batched shape checking to `LinearAlgebraUtils.h`
Pull Request resolved: pytorch#13453

Reviewed By: soumith

Differential Revision: D12942039

Pulled By: zou3519

fbshipit-source-id: 1b8007f00218e61593fc415865b51c1dac0b6a35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zou3519 zou3519 zou3519 approved these changes

+1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vishwakftw @zou3519 @facebook-github-bot @ezyang