MAINT:lstsq: Switch to tranposed problem if the array is tall #12206
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
pinging @sturlamolden and others. It is a relatively simple fix but a second pair of eyes won't hurt. |
|
Looks good |
|
It needs a testcase. |
|
Hmm I don't know how to test this actually since behavior is not changed. Any ideas anyone? |
|
Just compute the pinv for two arrays. Let they be above and below the threshold for transposition. Then compare with some other means of computing pinv, like cho_solve(cho_factor(A.T@A),np.eye(p))@A.T |
|
And since you are returning a transpose, it might also be possible to see if this happened by looking at the .flags and .strides for the returned pinv array. |
miladsade96
approved these changes
May 28, 2020
|
LGTM |
|
Reviews are positive here, CI is green, no old tests were modified, just a new test added, so merging. Thanks @ilayn and reviewers. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference issue
Closes #12196
What does this implement/fix?
The pseudo-inverse solution via scipy.linalg.pinv is through forming a least-squares problem AX=B where X is the pseudo-inverse of A and B is identity. When A is tall, necessarily B assumes the row number of A and is equally tall. However due to the properties of pseudo-inverse, one can also solve for A.T X.T = B and in this case B will be substantially smaller if A is substantially taller. This PR enables this behavior via an internal trans flag.
The switching point for assuming tallness is when row number is higher than column number by 10 percent. It is kind of arbitrary but derived from some numerical experiments. I would appreciate a more rigorous derivation of the switch point.