JuliaManifolds / Manifolds.jl

Manifolds.jl provides a library of manifolds aiming for an easy-to-use and fast implementation.
https://juliamanifolds.github.io/Manifolds.jl
MIT License
366 stars 52 forks source link

Multinomial randomness #702

Closed kellertuer closed 7 months ago

kellertuer commented 7 months ago

This PR introduces random for MultinomialSymmetric and MultinomialDoublyStochatstic, which I saw was missing due to a comment here.

Checking the corresponding paper https://ieeexplore.ieee.org/document/8861409 I saw they also introduce MultinoialSPD, which I added here as well. So this short side-track still needs

ToDo

The paper (its tech report we link in the docs to be precise) does state the Hessian for MultinomialDoublyStochastic, but it is quite involved (the symmetric one already is), that I am not sure I can “hack that down” in an efficient and stable way.

codecov[bot] commented 7 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (95e90c8) 99.57% compared to head (3b3b4e8) 99.57%.

Files Patch % Lines
.../manifolds/MultinomialSymmetricPositiveDefinite.jl 97.36% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #702 +/- ## ========================================== - Coverage 99.57% 99.57% -0.01% ========================================== Files 112 113 +1 Lines 10876 10961 +85 ========================================== + Hits 10830 10914 +84 - Misses 46 47 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kellertuer commented 7 months ago

All things I wanted to implement (and that needed fixing) are implemented (and fixed, e.g. the multinomalsymmetric ones have more than just the symmetric check for points now – again).

So this is no draft any longer, but still needs the tests to be checked and extended.

kellertuer commented 7 months ago

Multinomial is failing on 1.6, but it seems to not be so easy to reproduce, on my machine that works fine.

mateuszbaran commented 7 months ago

This is weird. is_vector at /ManifoldsBase/src/decorator_trait.jl:481 assumes that the error returned by check_vector has field val but ComponentManifoldError obviously doesn't have it. It looks like a problem with that is_vector perhaps?

kellertuer commented 7 months ago

Might also be that, but even more strange is, that there should not happen any error and on Julia 1.10, 1.9 and 1.6 on my machine it also does not happen.

mateuszbaran commented 7 months ago

Probably tolerance is too low. It would be more clear if that is_vector didn't try to access val or had a contingency in case it's not present.

kellertuer commented 7 months ago

Yes sure, that would help, but I do not yet directly see, why that is happening yet (also because I can not yet reproduce it here).

kellertuer commented 7 months ago

The problem seems to be that this line

https://github.com/JuliaManifolds/ManifoldsBase.jl/blob/65780dc51475b255258dfd35a225f09a0cd2810b/src/ManifoldsBase.jl#L769

either there or in the decorator at

https://github.com/JuliaManifolds/ManifoldsBase.jl/blob/65780dc51475b255258dfd35a225f09a0cd2810b/src/decorator_trait.jl#L481

implicitly assumes that the error at hand is a DomainError (or one of the other simple errors that has msg and val). So this is nothing to fix here, but maybe to rethink in ManifoldsBase. I am also not sure when this might occur, since for now I can not reproduce this on my machine. Here I for now tried to raise the tolerance instead.

edit: Ah I see if error is none the string is still build before returning false, so that might indeed happen, still – the best would be to build a string s in these two places we can warn/info about that is a bit less dependent on the internals of the error at hand.

edit: Ah we could change both lines and any other line with such access to use showerror instead. This is however still an issue for ManifoldsBase then.

kellertuer commented 7 months ago

Great! We have one false-positive, and now just need to write tests for the checks (one SPD matrix that is not multinomial, one actual point on M); similarly for the tangent space.

Maybe we could also improve the errors that are returned, since for now they would only display the wrong Manifold (i.e. that the point is not on the SPD manifold), which might be misleading.

kellertuer commented 7 months ago

For multinomial SPD I now wrote the tests – and had an idea for random tangents as well, we basically need tangent vectors to multinomial (rows & columns sum to zero) that are symmetric, so we could just pass that to Multinomial and symmetrize afterwards.

edit: ah no, that does not work. Then I do not have a good idea how to generate tangents on multinomalspd

kellertuer commented 7 months ago

Then I would also consider this to be ready for review and merge.

kellertuer commented 7 months ago

The one line we do not hit is a false-negative (sadly).

kellertuer commented 7 months ago

This one is now merged with #700, so we just have to wait for the tests.