ENH: orthogonal procrustes solver #3809
Conversation
|
See the discussion at #3786. |
| A = np.asarray_chkfinite(A) | ||
| B = np.asarray_chkfinite(B) | ||
| else: | ||
| A = np.asarray(A) |
There was a problem hiding this comment.
A = np.ascontiguousarray(A)
|
Hm, it seems np.dot always returns C order arrays. This will be better: A = np.asanyarray(A) |
|
or |
|
Is it cool to use |
|
The input to dot should be an ndarray, but it does not matter if it C or Fortran contiguous. But then the input to svd should be Fortran contiguous. Which means that those transpositions are actually preventing a transposition and a temporary copy inside the f2py layer... It's an f2py nastyness that transpose prevents transpose. |
|
Just to be clear, the concern about array order and contiguity in the procrustes implementation is for efficiency not correctness, right? |
|
It is mostly for saving memory |
|
Changes Unknown when pulling bcc46d8 on argriffing:linalg-orthogonal-procrustes into * on scipy:master*. |
|
But if you're in doubt, correctness always prewails. |
|
By the way, there is supposedly a better way to compute the svd of a matrix product -- Gene Golub et al. "Computing the SVD of a general matrix product/quotient." |
|
Changes Unknown when pulling bcc46d8 on argriffing:linalg-orthogonal-procrustes into * on scipy:master*. |
|
Changes Unknown when pulling 94cbac0 on argriffing:linalg-orthogonal-procrustes into * on scipy:master*. |
|
Added more tests and tried to improve memory efficiency according to the suggestions. |
|
Changes Unknown when pulling 94cbac0 on argriffing:linalg-orthogonal-procrustes into * on scipy:master*. |
|
To be more useful for applications, maybe the sum of singular values of the intermediate matrix should be returned? |
|
Changes Unknown when pulling 6602cf8 on argriffing:linalg-orthogonal-procrustes into * on scipy:master*. |
| Whether to check that the input matrices contain only finite numbers. | ||
| Disabling may give a performance gain, but may result in problems | ||
| (crashes, non-termination) if the inputs do contain infinities or NaNs. | ||
|
|
There was a problem hiding this comment.
Should there be a Raises section here?
I added this section explaining under what conditions a |
|
Changes Unknown when pulling 671ff41 on argriffing:linalg-orthogonal-procrustes into * on scipy:master*. |
|
@argriffing, thanks! 👍 |
| u, w, vt = svd(B.T.dot(A).T) | ||
| R = u.dot(vt) | ||
| # Always return R, and maybe return a scaling factor. | ||
| if compute_scale: |
There was a problem hiding this comment.
I'm not an immediate user of this feature, so take this with a spoonful of salt: are you sure you really want to have the number of returns to depend on the input? In my experience it often makes it harder to reason about the results and leads to a proliferation of if compute_scale clauses at call sites. In this case always returning the scale does not seem to cost much, what is the benefit of having a variable number of returns?
This is now removed. |
|
Changes Unknown when pulling 1195439 on argriffing:linalg-orthogonal-procrustes into * on scipy:master*. |
|
LGTM. |
ENH: orthogonal procrustes solver
This introduces a new feature rather than bug fixing or maintenance, so it's now open for the bike-shedding!