Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add method log_lbfgs - Riemannian logarithm using expm_frechet - #555

Closed
wants to merge 42 commits into from
Closed

add method log_lbfgs - Riemannian logarithm using expm_frechet - #555

wants to merge 42 commits into from

Conversation

dnguyend
Copy link

I added log_lbfgs - please let me know! I added new tests to test/manifolds/stiefel.jl. Additional tests are in this workbook and the jl files in this directory

@codecov
Copy link

codecov bot commented Nov 29, 2022 •
edited
Loading

Codecov Report

Merging #555 (74631b9) into master (48d6fa0) will decrease coverage by 0.02%.
The diff coverage is 98.54%.

@@            Coverage Diff             @@
##           master     #555      +/-   ##
==========================================
- Coverage   98.94%   98.92%   -0.03%     
==========================================
  Files         100      105       +5     
  Lines        9659    10473     +814     
==========================================
+ Hits         9557    10360     +803     
- Misses        102      113      +11
Impacted Files Coverage Δ
src/Manifolds.jl 100.00% <ø> (ø)
src/expm_frechet.jl 97.91% <97.91%> (ø)
src/minimize_lbfgs.jl 98.81% <98.81%> (ø)
src/manifolds/StiefelFrechet.jl 99.31% <99.31%> (ø)
src/manifolds/StiefelCanonicalMetric.jl 96.15% <0.00%> (-1.85%) ⬇️
src/manifolds/StiefelSubmersionMetric.jl 97.79% <0.00%> (-0.02%) ⬇️
src/manifolds/Sphere.jl 100.00% <0.00%> (ø)
src/tests/tests_general.jl 96.90% <0.00%> (ø)
src/groups/rotation_action.jl 100.00% <0.00%> (ø)
... and 8 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kellertuer
Copy link
Member

The first thing that is included, is, that your files have 0% code coverage, that you will have to check.

Concerning the format, the CI reports a lot of changes necessary:
https://github.com/JuliaManifolds/Manifolds.jl/actions/runs/3578455917/jobs/6018629102

But to fix that make sure
(a) to have Juliaformatter in its newest version
(b) to run the formatting from the base folder of Manifolds, e.g. as using JuliaFormatter; format("."), because we have a few setups for our formatting locally as well.

Besides that – wow! 600 lines of code for one function.

@dnguyend
Copy link
Author

dnguyend commented Nov 30, 2022 via email

@kellertuer
Copy link
Member

The other manifolds will work as soon as formatting is fine. As soon as a first of the tests fails, no other is started and since we have groups of tests, some were not run (see those with a gray (!) upfront in the list of the checks, those were not run) so as soon as those do run, the existing manifolds are covered (well 98.94% of their code).

Those new functions I will have to check, especially dot_exp is already provided I think.

@dnguyend
Copy link
Author

dnguyend commented Nov 30, 2022 via email

@kellertuer
Copy link
Member

Well if you have Manifolds on you machine in development mode, you can always also test your code locally with ] test Manifolds. But even if you only let the CI here check – that is still reasonably silent ;)

@dnguyend
Copy link
Author

dnguyend commented Nov 30, 2022 via email

@dnguyend
Copy link
Author

dnguyend commented Nov 30, 2022 via email

@dnguyend
Copy link
Author

dnguyend commented Nov 30, 2022 via email

@kellertuer
Copy link
Member

Formatting seems fine (you do not have to write a comment for every commit).

Concerning the tests, well here in the CI we aim to have an – maybe small in dimension but – test that covers every line and still performs something we can check to be valid/correct. This doe snot say we do not have bugs, but (a) minimises the number of bugs, since you have to think about code lines a second time, namely what you expect as an outcome an write a test for that, but even more
(b) the automatic tests (with high test coverage) make It easier to fix bugs and modify/improve the code, because this way we can be sure it still “behaves as expected”.

@dnguyend
Copy link
Author

dnguyend commented Nov 30, 2022 via email

@dnguyend
Copy link
Author

dnguyend commented Nov 30, 2022 via email

@kellertuer
Copy link
Member

kellertuer commented Dec 1, 2022 •
edited
Loading

Work further locally on tests and documentation until

JuliaPy/Conda.jl#229

is addressed / fixed :)

@dnguyend
Copy link
Author

dnguyend commented Dec 2, 2022 via email

@kellertuer
Copy link
Member

instead of modifying runtest you can also just include that file yourself. We wrote the single manifold tests in a way that they also work standalone, that is only with include, for my workflow it would be great if you could comment in the points raised above when you have solved them (of course also when you have a question or would like to not approach something I asked about).

Code coverage for a little bit better, but we are not yet there.

@dnguyend
Copy link
Author

dnguyend commented Dec 4, 2022

Thanks. I added replies to the issues you raised above.

I have been running test on single files. I would like to know the syntax to run one file and generate the coverage file, the .cov file generated next to the code showing what line of code is not covered, like these:

[ Info: CoverageTools.process_file: Detecting coverage for src/manifolds/StiefelEuclideanMetric.jl
[ Info: CoverageTools.process_cov: processing src/manifolds/StiefelEuclideanMetric.jl.26760.cov
[ Info: CoverageTools.process_file: Detecting coverage for src/manifolds/StiefelFrechet.jl
[ Info: CoverageTools.process_cov: processing src/manifolds/StiefelFrechet.jl.26760.cov
[ Info: CoverageTools.process_file: Detecting coverage for src/manifolds/StiefelSubmersionMetric.jl
[ Info: CoverageTools.process_cov: processing src/manifolds/StiefelSubmersionMetric.jl.26760.cov

Pkg.test("Manifolds", coverage=true) generates the .cov files, they are useful for me, but I had to modify runtests.jl to trick it to work - so it would be great to have a syntax to do it directly.

I am able to get coverage for expm_frechet.jl and StiefelFrechet.jl to near 98%, only the views are now flagged as uncovered. I don't know how to fix the views - presumably there is an ignore option in the setup file ?

expm_frechet.jl
StiefelFrechet.jl

I haven't started on minimize_lbfgs.jl, this is a bit tricky as it is a general purpose optimizer - there are many scenarios - but I am working on it.

@kellertuer
Copy link
Member

I never did code cos for single files, since I usually work through all points of coverage and commit and look ad code con here.

Just quickly, if its relevant ( will add to the doc), the magic numbers on
expm_frechet comes from pade approximation (copied from scipy.linalg)

Concerning this point – hm I am not much a fan of including code from other packages here, but if that is necessary, then relay only with a lot of documentation and notes where that is from (and a check of whether their License allows that of course)

Concerning the @views that are false positives and they will sadly stay (actually these false positives are the reason we are not at 100% Code Coverage

minimize_lbfgs.jl, this is a bit tricky as it is a general purpose optimizer

but did you write that yourself? Otherwise – and for code even more than for constants – I would be very unhappy to copy over code from other packages - and maybe if you write a more specialised solver it might both be faster and easier to test?

@dnguyend
Copy link
Author

dnguyend commented Dec 4, 2022 via email

@kellertuer
Copy link
Member

No worries, I am aware that you are in the process, I am just trying in a few steps to give feedback early to reduce your work (hopefully at least ;) )

@dnguyend
Copy link
Author

dnguyend commented Dec 5, 2022

The new push should bring the 3 files I work on much closer to target, but if there are still issues I will investigate.

I added 4 functions out in the docs, expm_frechet, minimize (from minimize_lbfgs), dot_exp and log_lbfgs. The helper functions have comments in docstring format but does not show on the docs.
Thx

@dnguyend
Copy link
Author

dnguyend commented Dec 5, 2022 via email

@kellertuer
Copy link
Member

The first thing I see is that your test runs issue quite some output. That should never be the case, especially not something like

i=	27 n=	93 k=	15

besides that maybe one of the reports (we gather 2, one for Lie groups one for manifolds) came too late (before this PR the manifolds test case ran for about an hour, your last test run was more than 1.5 hours it seems which is really long).
So for now I am not 100% sure how to continue since it seems something in your tests causes the second report not to be reported or being reported too late.

@dnguyend
Copy link
Author

dnguyend commented Dec 5, 2022 via email

@kellertuer
Copy link
Member

Please do not do random tests – that is absolutely horrible to debug if one of the random cases fails, because it is super hard to debug – for any maintainer. I have such a case for about a year now I another package and was not yet able to reproduce the case in a non-random setting to finally fix it.

So: Please no random stuff in the tests.

kellertuer
kellertuer reviewed Dec 5, 2022
View reviewed changes
src/expm_frechet.jl
if isnothing(s)
# scaling
s = max(0, Int(ceil(log2(A_norm_1 / ell_table_61[14]))))
# pade order 13
Copy link
Member

Choose a reason for hiding this comment

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

What is the goal of this comment if the next line calls a function of same name? And why are there so many other diff_pade13 functions? To more this file is a little chaotic and most of its content a little strange – sorry, but it is really mainly the problem that I will have to be able to maintain that and in its current form, this is just not possible.

Copy link
Author

Choose a reason for hiding this comment

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

will clean up - and make diff_pade13 calling diff_pade13!

kellertuer
kellertuer reviewed Dec 5, 2022
View reviewed changes
src/manifolds/StiefelFrechet.jl
t,
) where {n,k}
q = similar(p)
dot_q = similar(p)
Copy link
Member

Choose a reason for hiding this comment

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

I think dot_q is a tangent vector, right? First of all, then dot_qshould be called diff_exp or differential_exp but even more, dot_q should be initialised as a tangent vector (zero_vector(M,p) probably) and better be called X or Y following our naming convention.

Copy link
Author

Choose a reason for hiding this comment

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

OK will do

@kellertuer
Copy link
Member

Since I run into all these problems when I just take a 5 minute look at your code, I am slowly wondering how much time (and comments) it would take me (or the other maintainers) to take a look at more than a few lines of code and whether all the comments can be solved. Maybe it would be a better Idea if you do a small package yourself (based on ManifoldsBase) that implements this part?
You could also make your small package depend in Manifolds if you want to use Stiefel and the Metric type.

The advantage that way is

  • you can choose any dependencies you like (which might make your code smaller, though with more dependencies)
  • You can choose a nice name
  • We would reference to that In our Stiefel Docs,
  • just loading both packages (Manifolds and yours) would extend Manifolds nicely.
  • You can even decide yourself how much testing you want to do, and documentation and such.

What do you think?

@dnguyend
Copy link
Author

dnguyend commented Dec 5, 2022

Thanks - it is kind of unexpected - thanks for the suggestion. I will have to think about working on a new package - that will likely to take more time commitment than I can afford.

By the way, I would like to share a paper Operator-valued formulas for Riemannian Gradient and Hessian and families of tractable that shows how to compute the Levi-Civita connection for most metrics, including Stiefel/Grassmann/Flag manifolds, semi-definite matrix manifold with a complete metric. I believe it also gives the connection for the Symplectic Stiefel manifold that is in your package.
Best,
Du

@kellertuer
Copy link
Member

Thanks for the link.

I am sorry that that will take more time, but keep in mind I am doing this in mz free time and have spent so much time on this already, and barely looked at about 20-30 lines of code. In that speed this PR is done (given that I spent an hour a day or such, like I did the last week here) this will take a very very very long time with a lot of time from my side. And sorry, I can not do that besides lectures, master and phd students and my own research.

Besides that there are the questions with licensing that I am still not sure about since you did not answer that completely yet. In your own package – I would not care about your licensing – in mine I do.

@kellertuer
Copy link
Member

I will close this for now. Let me know if you need help with the additional package. This also does not mean we will never include this here, I just feel that it will need some time to mature.

And as said, as you own package, you can depend on Optim, Manifolds, and whatever you want, so your first code here should be good to go for such a package :)

@kellertuer kellertuer closed this Dec 12, 2022
@dnguyend
Copy link
Author

dnguyend commented Dec 12, 2022 via email

@kellertuer
Copy link
Member

As I mentioned – I can currently not provide the time to help you finish this, especially

  • documentation
  • testing
  • review all code lines and the design of the functions.
  • Check dependencies and especially Licensing of code you include

and there seems to be no one else as well. If you at some point have these criteria fulfilled – feel free to reopen this PR or do a new one. But I feel starting from a separate package is really the best way to go here. An we are happy to link your project then.

@dnguyend
Copy link
Author

dnguyend commented Dec 12, 2022 via email

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

@kellertuer kellertuer kellertuer left review comments

Assignees
No one assigned
Labels
preview docs Add this label if you want to see a PR-preview of the documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dnguyend @kellertuer