search

JuliaManifolds / ManifoldsBase.jl

Basic interface for manifolds in Julia
https://juliamanifolds.github.io/ManifoldsBase.jl/
MIT License
83 stars 8 forks source link

Simplify the tutorial and move to quarto. #158

Closed kellertuer closed 1 year ago

kellertuer commented 1 year ago

This PR simplifies the existing tutorial on how to define a manifold and moves it to a quarto notebooks as well.

I noticed two things this PR could also resolve

Besides that this needs a careful check that the new CI runs fine and that all examples and references are correct. The example itself was not changed much.

ToDo

codecov[bot] commented 1 year ago

Codecov Report

Merging #158 (2f89cff) into master (2254fe8) will decrease coverage by 0.08%. The diff coverage is n/a.

:exclamation: Current head 2f89cff differs from pull request most recent head bad9c22. Consider uploading reports for the commit bad9c22 to get more accurate results

@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
- Coverage   99.92%   99.84%   -0.08%     
==========================================
  Files          19       19              
  Lines        2538     2537       -1     
==========================================
- Hits         2536     2533       -3     
- Misses          2        4       +2
Impacted Files Coverage Δ
src/metric.jl 100.00% <ø> (ø)
src/retractions.jl 100.00% <ø> (ø)

... and 2 files with indirect coverage changes

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

kellertuer commented 1 year ago

A future idea would be to do a tutorial for traits with an embedding idea, and maybe one for metrics or such.

kellertuer commented 1 year ago

Hm, locally everything works, but the documenter CI says IJulia is missing the 1.9 kernel? Not yet sure why...the first set seems to be a bit difficult every now and then of quarto+Julia maybe.

kellertuer commented 1 year ago

Hm, I am a bit lost here, all config files are exactly the same as for Manopt.jl and IJulia us built directly before we switch to quarto, still IJulia seems to not have Julia 1.9 in its kernels. To me a total mystery.

kellertuer commented 1 year ago

Further investigations, directly before it says the kernel is not present it lists the kernel as present see the currently added warning at https://github.com/JuliaManifolds/ManifoldsBase.jl/actions/runs/5431263051/jobs/9877519967#step:9:709

kellertuer commented 1 year ago

Finally managed to set up the CI correctly. Now the only thing left is to find out why @refs in the tutorial do not yet work.

kellertuer commented 1 year ago

This is finished besides the discussion on the default difference between retract and exp

mateuszbaran commented 1 year ago

Sure, I think this can use Quarto as well.

  • it seems for exp it is enough to define exp!(M, q, p, X) the exp!(M, q, p, X, t) falls back to this. for retract it is the other way around, one has to define retract_method!(M, q, p, X, t)

IIRC I wanted to make exp!(M, q, p, X) fall back to exp!(M, q, p, X, t) but it would be technically a braking change so we are in a somewhat inconsistent state now.

  • I noticed that the method to implement for a retraction is not always clear, we could add such a note to each AbstractRetractionMethod subtype which lower level function corresponds to the high level type?

Sure, that is a good idea.

kellertuer commented 1 year ago

But can we fix that without breaking, that is having both work the same?

This is not so urgent since it mainly affects people implementing manifolds, not using them, but it would be great to have a unified interface.

For the Retraction types, I will add that to the docs then.

kellertuer commented 1 year ago

I added a note to all retractions and inverse retractions which functions are related to them. I also moved the discussion about exp/retract to a new issue – and did a thorough check about the current state there.

So this PR should be ready to go :)

mateuszbaran commented 1 year ago

But can we fix that without breaking, that is having both work the same?

As far as I know, we can't.

kellertuer commented 1 year ago

Well, then with breaking ;) Still unifying them would help users I think.

mateuszbaran commented 1 year ago

I agree, we should unify them next time we make a breaking change here.

mateuszbaran commented 1 year ago

image There seems to be something wrong with formulas in the new tutorial.

kellertuer commented 1 year ago

Then let's find a good unification in the issue and do that before the next breaking change :)

kellertuer commented 1 year ago

Ups, it seems I missed that \mid was not recognised. should be fixed now (i.e. works locally now).

kellertuer commented 1 year ago

THanks, since this does not introduce a new feature is it ok if we leave it on master until the next feature comes along? Or should I register this as a new version?

mateuszbaran commented 1 year ago

I think it's better to register since stable docs will then include this change.