Skip to content

Conversation

@vishwakftw
Copy link
Contributor

@vishwakftw vishwakftw commented Mar 25, 2019

Changelog:

  • Renames btrifact and btrifact_with_info to luto remain consistent with other factorization methods (qr and svd).
  • Now, we will only have one function and methods named lu, which performs lu decomposition. This function takes a get_infos kwarg, which when set to True includes a infos tensor in the tuple.
  • Rename all tests, fix callsites
  • Create a tentative alias for lu under the name btrifact and btrifact_with_info, and add a deprecation warning to not promote usage.
  • Add the single batch version for lu so that users don't have to unsqueeze and squeeze for a single square matrix (see changes in determinant computation in LinearAlgebra.cpp)

Test Plan:

  • All tests should pass to confirm that the patch is correct.

Copy link
Collaborator

@ssnl ssnl left a comment

Choose a reason for hiding this comment

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

Nice! I would say that this is a good time to get rid of lu_with_info API, and use a keyword arg (e.g., get_info=False) of lu to dispatch to different method in backend.

@vishwakftw
Copy link
Contributor Author

vishwakftw commented Mar 26, 2019

I removed my earlier comment due to some discrepancy.

I think that removing lu_with_info for a keyword arg sounds good.

and use a keyword arg (e.g., get_info=False) of lu to dispatch to different method in backend.

Are you suggesting overriding the behavior of lu as done for unique in functional.py? Doing this might risk visibility of lu_with_info in ATen (and consequently in the C++ API)

@vishwakftw
Copy link
Contributor Author

Otherwise, we could always return infos (in a 3-tuple: LU, pivots, infos) in the C++ API, but build the output tuple in Python depending on the value of get_infos.

@ezyang
Copy link
Contributor

ezyang commented Mar 26, 2019

Test failure:

Mar 26 06:53:23 ======================================================================
Mar 26 06:53:23 FAIL: test_lu (__main__.TestCuda)
Mar 26 06:53:23 ----------------------------------------------------------------------
Mar 26 06:53:23 Traceback (most recent call last):
Mar 26 06:53:23   File "/var/lib/jenkins/workspace/test/common_utils.py", line 318, in wrapper
Mar 26 06:53:23     method(*args, **kwargs)
Mar 26 06:53:23   File "/var/lib/jenkins/workspace/test/common_utils.py", line 129, in wrapper
Mar 26 06:53:23     fn(*args, **kwargs)
Mar 26 06:53:23   File "test_cuda.py", line 2366, in test_lu
Mar 26 06:53:23     _TestTorchMixin._test_lu(self, lambda t: t.cuda())
Mar 26 06:53:23   File "/var/lib/jenkins/workspace/test/test_torch.py", line 1774, in _test_lu
Mar 26 06:53:23     run_test(ms, batch, cast)
Mar 26 06:53:23   File "/var/lib/jenkins/workspace/test/test_torch.py", line 1768, in run_test
Mar 26 06:53:23     self.assertIsNone(nopiv)
Mar 26 06:53:23 AssertionError: tensor([0, 0, 0], device='cuda:0', dtype=torch.int32) is not None
Mar 26 06:53:23 
Mar 26 06:53:23 ----------------------------------------------------------------------

@vishwakftw
Copy link
Contributor Author

My bad, fixing it now.

@vishwakftw
Copy link
Contributor Author

I have a local patch that removes the lu_with_info function and add the get_infos argument in lu. Should I push that or just the fix for the test failures?

@ezyang
Copy link
Contributor

ezyang commented Mar 27, 2019

@vishwakftw If you like, you can stack the diffs using https://github.com/ezyang/ghstack

Otherwise, do what is easiest for you (but with the caveat that the bigger the diff, the harder it is for us to review ;)

Changelog:

- Renames `btrifact` and `btrifact_with_info` to `lu` respectively to remain consistent with other factorization methods (`qr` and `svd`).
- Now, we will only have one function and methods named `lu`, which performs `lu` decomposition. This function takes a get_infos kwarg, which when set to True includes a infos tensor in the tuple.
- Rename all tests, fix callsites
- Create a tentative alias for `lu` under the name `btrifact` and `btrifact_with_info`, and add deprecation warnings to not promote usage.
- Add the single batch version for `lu` so that users don't have to unsqueeze and squeeze for a single square matrix (see changes in determinant computation in `LinearAlgebra.cpp`)

Test Plan:

- All tests should pass to confirm that the patch is correct.
@vishwakftw vishwakftw changed the title Rename btrifact* to lu* Rename btrifact* to lu Mar 27, 2019
@vishwakftw vishwakftw force-pushed the btrifact-to-lu_factor branch from cf4d515 to 9aa2827 Compare March 27, 2019 15:42
@vishwakftw vishwakftw added the ready for review (this tag is deprecated) All PRs are ready for review unless they are draft, WIP, or have undismissed requested changes label Mar 27, 2019
@vishwakftw
Copy link
Contributor Author

@ezyang The patch that I was referring to was rather small, which is why I've amended the commit directly. I'll use ghstack for upcoming PRs (renaming btrisolve and btriunpack).

@vishwakftw
Copy link
Contributor Author

@pytorchbot rebase this please

@vishwakftw vishwakftw force-pushed the btrifact-to-lu_factor branch from e4da3d7 to 17d9cda Compare March 28, 2019 05:02
@vishwakftw
Copy link
Contributor Author

Jenkins tests fail with this error:

06:50:11 Running test_docs_coverage ... [2019-03-28 06:50:11.781583]
06:50:11 test_tensor (test_docs_coverage.TestDocCoverage) ... ok
06:50:11 test_torch (test_docs_coverage.TestDocCoverage) ... FAIL
06:50:11 
06:50:11 ======================================================================
06:50:11 FAIL: test_torch (test_docs_coverage.TestDocCoverage)
06:50:11 ----------------------------------------------------------------------
06:50:11 Traceback (most recent call last):
06:50:11   File "/var/lib/jenkins/workspace/test/test_docs_coverage.py", line 67, in test_torch
06:50:11     don't want to document?''')
06:50:11 AssertionError: 
06:50:11 List of functions documented in torch.rst and in python are different.
06:50:11 Do you forget to add new thing to torch.rst, or whitelist things you
06:50:11 don't want to document?
06:50:11 
06:50:11 ----------------------------------------------------------------------
06:50:11 Ran 2 tests in 0.003s
06:50:11 
06:50:11 FAILED (failures=1)

CircleCI tests pass instead:

Mar 28 05:31:23 Running test_docs_coverage ... [2019-03-28 05:31:23.569080]
Mar 28 05:31:23 test_tensor (__main__.TestDocCoverage) ... ok
Mar 28 05:31:23 test_torch (__main__.TestDocCoverage) ... ok
Mar 28 05:31:23 
Mar 28 05:31:23 ----------------------------------------------------------------------
Mar 28 05:31:23 Ran 2 tests in 0.004s
Mar 28 05:31:23 
Mar 28 05:31:23 OK

@vishwakftw
Copy link
Contributor Author

@pytorchbot rebase this please

@vishwakftw
Copy link
Contributor Author

@ezyang @ssnl is this good to go?

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.

@soumith is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@vishwakftw vishwakftw deleted the btrifact-to-lu_factor branch March 29, 2019 07:46
zdevito pushed a commit to zdevito/ATen that referenced this pull request Mar 29, 2019
Summary:
Changelog:

- Renames `btrifact` and `btrifact_with_info` to `lu`to remain consistent with other factorization methods (`qr` and `svd`).
- Now, we will only have one function and methods named `lu`, which performs `lu` decomposition. This function takes a get_infos kwarg, which when set to True includes a infos tensor in the tuple.
- Rename all tests, fix callsites
- Create a tentative alias for `lu` under the name `btrifact` and `btrifact_with_info`, and add a deprecation warning to not promote usage.
- Add the single batch version for `lu` so that users don't have to unsqueeze and squeeze for a single square matrix (see changes in determinant computation in `LinearAlgebra.cpp`)
Pull Request resolved: pytorch/pytorch#18435

Differential Revision: D14680352

Pulled By: soumith

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

@soumith merged this pull request in d859031.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open source ready for review (this tag is deprecated) All PRs are ready for review unless they are draft, WIP, or have undismissed requested changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants