Implement the canonical metric on the Stiefel manifold #335

kellertuer · 2021-03-06T17:32:32Z

This PR adds a second metric to the Stiefel Manifold that provides both a exp and a log map.

check documentation (I seem to have locally a problem with Plots/PyPlot)
implement tests

…nical metric on manifolds.

sethaxen

I haven't looked at this super closely, but I had some suggestions to reduce allocations and improve efficiency

src/manifolds/StiefelCanonicalMetric.jl

src/manifolds/StiefelEuclideanMetric.jl

src/manifolds/StiefelCanonicalMetric.jl

Co-authored-by: Seth Axen <[email protected]>

src/manifolds/StiefelEuclideanMetric.jl

src/manifolds/StiefelCanonicalMetric.jl

Co-authored-by: Seth Axen <[email protected]>

…ces a bug (see JuliaPlots/Plots.jl#3341) in the next version

kellertuer · 2021-03-08T09:06:03Z

Hm, setting plots to "1.6, =1.10.5" loads for the extra of Plots` in the tests - ... 1.10.6. Here locally tests run fine with 1.10.5, on Julia 1.4 this loads the last working version (1.6), but for Julia 1.5 – or maybe in general? compat constraints for extra test packages seem to be ignored.

Similarly, I can also not convince the documenters Project.toml to respect this idea.

codecov · 2021-03-08T10:33:42Z

Codecov Report

Merging #335 (07d8598) into master (b3d7787) will decrease coverage by 0.01%.
The diff coverage is 97.75%.

@@            Coverage Diff             @@
##           master     #335      +/-   ##
==========================================
- Coverage   97.63%   97.62%   -0.02%     
==========================================
  Files          74       76       +2     
  Lines        5794     5848      +54     
==========================================
+ Hits         5657     5709      +52     
- Misses        137      139       +2

Impacted Files	Coverage Δ
src/Manifolds.jl	`100.00% <ø> (ø)`
src/manifolds/Stiefel.jl	`99.21% <ø> (-0.16%)`	⬇️
src/manifolds/StiefelEuclideanMetric.jl	`97.61% <97.61%> (ø)`
src/manifolds/StiefelCanonicalMetric.jl	`97.77% <97.77%> (ø)`
src/manifolds/SymmetricPositiveDefinite.jl	`100.00% <100.00%> (ø)`
...anifolds/SymmetricPositiveSemidefiniteFixedRank.jl	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3d7787...07d8598. Read the comment docs.

…es testing.

kellertuer · 2021-03-08T13:12:37Z

I just checked your code – at one point the small/economic Q or QR was expanded to the full one, which I would avoid using a view – and I forgot one line of code.

Now testing should work and cover all new functions.

kellertuer · 2021-03-08T16:30:47Z

I am not yet sure, whether the inner for loop is now correct. In the old form I

forgot about the counter
we lost some updates in the code optimization
I think we have to do this in complex numbers

Though I am still looking for an example that

is not finished before the door loop (currently our examples directly have a norm of C less than 1e-15)
is within objectivity radius and indeed does invert exp.

I wrote the author of the algorithm about that last point.

src/manifolds/StiefelCanonicalMetric.jl

test/stiefel.jl

Co-authored-by: Mateusz Baran <[email protected]>

kellertuer · 2021-03-12T07:27:52Z

What is "warst"?

some strange autocorrect (German past participle of “sein” - to be) who I meant to type kwargs.

mateuszbaran · 2021-03-12T09:16:49Z

My only issue with introducing that on a generic level is that last time I checked Julia did a relatively poor job at optimizing out kwargs. Maybe it's better now, I'll have to check it carefully before this change is made.

kellertuer · 2021-03-12T09:18:22Z

Sure. I am also not 100% sure a global level would be that good, also for further usage (in algorithms say). We can keep it the current way for not, surely.

mateuszbaran · 2021-03-12T10:31:31Z

Regarding the failure on Julia 1.4, I think it's fine to check ambiguities only on Julia >=1.5. By the way, Julia nightly also has a similar problem: JuliaLang/julia#37711 .

kellertuer · 2021-03-12T10:33:47Z

Ok, I till change that as soon as I find time to look for the remaining code lines to be covered.

sethaxen · 2021-03-12T23:44:51Z

Perhaps unrelated, but the ambiguity test stackoverflows for me on Windows only on Julia 1.5: https://github.com/JuliaManifolds/Manifolds.jl/pull/249/checks?check_run_id=2093697330#step:5:329.

kellertuer · 2021-03-13T05:22:33Z

Yes, here in the checks the same happens for Julia 1.4, seems to be unrelated, though I have not yet seen how to fix that.

kellertuer · 2021-03-18T08:52:15Z

So with the commit from today this should be feature complete and all new lines tested. If you could take another look and check if all changes are good and the changes are good, that would be great.

Concerning the avoidance of allocations, the only thing I had to revert is the use of svd! (back to svd) since the input matrix is still needed afterwards.

sethaxen

Just a few comments and a few more allocation-reductions. There are more allocations that can be removed using e.g. qr! but I don't think they'll have a huge impact on performance, and it's less likely that qr! will be overloaded for some special matrix than qr will be.

docs/src/manifolds/stiefel.md

src/manifolds/StiefelCanonicalMetric.jl

sethaxen · 2021-03-21T01:23:12Z

src/manifolds/StiefelCanonicalMetric.jl

+    n == k && return mul!(q, p, exp(A))
+    QR = qr(X - p * A)
+    BC_ext = exp([A -QR.R'; QR.R 0*I])
+    q .= [p Matrix(QR.Q)] * @view(BC_ext[:, 1:k])


If you blockwise multiply here, then instead of the 3 allocations here, I think you can do with none.

How would I do that? I twiddled so much with these few lines of code, before and after your allocation reduction (about 2 days to find that we can not use the svd!) that I don't see directly how to do this block wise.

I'll take a stab at it.

src/manifolds/StiefelCanonicalMetric.jl

src/manifolds/SymmetricPositiveDefinite.jl

Co-authored-by: Seth Axen <[email protected]>

…aManifolds/Manifolds.jl into kellertuer/steifel-canonical-metric

kellertuer · 2021-03-21T08:59:34Z

Just a few comments and a few more allocation-reductions. There are more allocations that can be removed using e.g. qr! but I don't think they'll have a huge impact on performance, and it's less likely that qr! will be overloaded for some special matrix than qr will be.

I don't feel comfortable currently trying that after it took me two full days changing to svd! to see that it had too many side effects and made the calculation wrong, since the input matrix was still used afterwards. So my fear is not the overwriting but the side effects from working in place. Debugging this log! here is not really much fun. So if you want to give it a try, feel free to do so. I would like to not do that, to not again take a few full days getting log! back up working (possibly in the form it is now).

kellertuer · 2021-03-21T12:57:28Z

Should we maybe deactivate the windows test or at least ambiguities thereon until it has come to conclusion not to do stack overflows when looking for those?

sethaxen · 2021-03-22T06:48:25Z

Just a few comments and a few more allocation-reductions. There are more allocations that can be removed using e.g. qr! but I don't think they'll have a huge impact on performance, and it's less likely that qr! will be overloaded for some special matrix than qr will be.

I don't feel comfortable currently trying that after it took me two full days changing to svd! to see that it had too many side effects and made the calculation wrong, since the input matrix was still used afterwards. So my fear is not the overwriting but the side effects from working in place. Debugging this log! here is not really much fun. So if you want to give it a try, feel free to do so. I would like to not do that, to not again take a few full days getting log! back up working (possibly in the form it is now).

Oh, I meant to say that these changes are probably not worth the code complexity or effort. I'm sorry for not being clear.

sethaxen · 2021-03-22T06:49:32Z

Should we maybe deactivate the windows test or at least ambiguities thereon until it has come to conclusion not to do stack overflows when looking for those?

This is probably a good idea.

sethaxen

Assuming tests pass and you approve of the changes, I think this is ready to merge. Nice work!

…aManifolds/Manifolds.jl into kellertuer/steifel-canonical-metric

kellertuer added 2 commits March 6, 2021 17:05

reorganize code and sketch initial algorithms of exp and log for cano…

03ebd9f

…nical metric on manifolds.

update documentation.

1908eb6

kellertuer added the preview docs Add this label if you want to see a PR-preview of the documentation label Mar 6, 2021

undo a commented our part in docs.

e79a1b5

sethaxen requested changes Mar 7, 2021

View reviewed changes

Apply suggestions from code review

c993f3b

Co-authored-by: Seth Axen <[email protected]>

sethaxen reviewed Mar 7, 2021

View reviewed changes

src/manifolds/StiefelEuclideanMetric.jl Outdated Show resolved Hide resolved

src/manifolds/StiefelCanonicalMetric.jl Outdated Show resolved Hide resolved

sethaxen reviewed Mar 8, 2021

View reviewed changes

src/manifolds/StiefelCanonicalMetric.jl Show resolved Hide resolved

kellertuer and others added 5 commits March 8, 2021 07:28

Apply suggestions from code review

f36ab3e

Co-authored-by: Seth Axen <[email protected]>

runs formatter.

edc96c9

Fix plots (for documentation and testing) to 1.10.5, since it introdu…

e7b914e

…ces a bug (see JuliaPlots/Plots.jl#3341) in the next version

Losen constraints for Julia 1.4

3950004

fix latex equation.

d9008f3

kellertuer added 2 commits March 8, 2021 10:19

Fix documentation.

9c61049

changing compats again, hopefully this works?

0a8aae0

kellertuer added 2 commits March 8, 2021 14:04

starts testing.

f97f37f

adopts one view, such that the multiplication is well defined. Finish…

cee6df2

…es testing.

kellertuer added 3 commits March 8, 2021 15:02

Fix spacing.

018c288

fixes a few bugs, starts extended testing that includes the iterations.

c89c763

runs formatter.

b062c87

kellertuer added 3 commits March 8, 2021 18:11

removes a little bit of unnecessary lines.

50f83ca

Merge branch 'master' into kellertuer/steifel-canonical-metric

ec7f6f3

enforce real part of log and avoid svd! to not change V.

4d359b5

kellertuer added the Ready-for-Review A label for pull requests that are feature-ready label Mar 11, 2021

mateuszbaran reviewed Mar 11, 2021

View reviewed changes

src/manifolds/StiefelCanonicalMetric.jl Outdated Show resolved Hide resolved

test/stiefel.jl Outdated Show resolved Hide resolved

test/stiefel.jl Show resolved Hide resolved

Update src/manifolds/StiefelCanonicalMetric.jl

7103ada

Co-authored-by: Mateusz Baran <[email protected]>

improve the test, where an approximation is actually needed.

94302ca

sethaxen requested changes Mar 21, 2021

View reviewed changes

kellertuer and others added 3 commits March 21, 2021 09:41

Update src/manifolds/SymmetricPositiveDefinite.jl

b13cfb0

Co-authored-by: Seth Axen <[email protected]>

address points from code review.

9b8e3c4

Merge branch 'kellertuer/steifel-canonical-metric' of github.com:Juli…

571776a

…aManifolds/Manifolds.jl into kellertuer/steifel-canonical-metric

Adress points from code review.

456621e

Merge branch 'master' into kellertuer/steifel-canonical-metric

7f99f8d

sethaxen added 6 commits March 22, 2021 00:23

Multiply blockwise

ca261fd

Use convert to avoid copy

2b1ebd0

Use log_safe!

34eba33

Avoid unnecessary copy

d7ce927

Multiply blockwise to reduce allocations

0ceb65b

Increment version number

296f107

sethaxen approved these changes Mar 22, 2021

View reviewed changes

kellertuer added 2 commits March 22, 2021 10:35

remove windows from testing temporarily.

2a2d8e7

Merge branch 'kellertuer/steifel-canonical-metric' of github.com:Juli…

07d8598

…aManifolds/Manifolds.jl into kellertuer/steifel-canonical-metric

kellertuer merged commit f6b302a into master Mar 22, 2021

kellertuer deleted the kellertuer/steifel-canonical-metric branch June 4, 2021 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the canonical metric on the Stiefel manifold #335

Implement the canonical metric on the Stiefel manifold #335

kellertuer commented Mar 6, 2021 •

edited

Loading

sethaxen left a comment

kellertuer commented Mar 8, 2021 •

edited

Loading

codecov bot commented Mar 8, 2021 •

edited

Loading

kellertuer commented Mar 8, 2021

kellertuer commented Mar 8, 2021

kellertuer commented Mar 12, 2021

mateuszbaran commented Mar 12, 2021

kellertuer commented Mar 12, 2021

mateuszbaran commented Mar 12, 2021

kellertuer commented Mar 12, 2021

sethaxen commented Mar 12, 2021

kellertuer commented Mar 13, 2021

kellertuer commented Mar 18, 2021

sethaxen left a comment

sethaxen Mar 21, 2021

kellertuer Mar 21, 2021

sethaxen Mar 22, 2021

kellertuer commented Mar 21, 2021

kellertuer commented Mar 21, 2021

sethaxen commented Mar 22, 2021

sethaxen commented Mar 22, 2021

sethaxen left a comment

Implement the canonical metric on the Stiefel manifold #335

Implement the canonical metric on the Stiefel manifold #335

Conversation

kellertuer commented Mar 6, 2021 • edited Loading

sethaxen left a comment

Choose a reason for hiding this comment

kellertuer commented Mar 8, 2021 • edited Loading

codecov bot commented Mar 8, 2021 • edited Loading

Codecov Report

kellertuer commented Mar 8, 2021

kellertuer commented Mar 8, 2021

kellertuer commented Mar 12, 2021

mateuszbaran commented Mar 12, 2021

kellertuer commented Mar 12, 2021

mateuszbaran commented Mar 12, 2021

kellertuer commented Mar 12, 2021

sethaxen commented Mar 12, 2021

kellertuer commented Mar 13, 2021

kellertuer commented Mar 18, 2021

sethaxen left a comment

Choose a reason for hiding this comment

sethaxen Mar 21, 2021

Choose a reason for hiding this comment

kellertuer Mar 21, 2021

Choose a reason for hiding this comment

sethaxen Mar 22, 2021

Choose a reason for hiding this comment

kellertuer commented Mar 21, 2021

kellertuer commented Mar 21, 2021

sethaxen commented Mar 22, 2021

sethaxen commented Mar 22, 2021

sethaxen left a comment

Choose a reason for hiding this comment

kellertuer commented Mar 6, 2021 •

edited

Loading

kellertuer commented Mar 8, 2021 •

edited

Loading

codecov bot commented Mar 8, 2021 •

edited

Loading