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.

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

Simplify the tutorial and move to quarto. #158

Merged
merged 19 commits into from
Jul 2, 2023

Conversation

kellertuer
Copy link
Member

@kellertuer kellertuer commented Jun 30, 2023

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

  • 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)
  • 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?

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

  • references in the new example seem to not yet work (the @refs) I am not 100% sure why since I made that the same way as over on the other packages.

@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

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

❗ 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

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

@kellertuer
Copy link
Member Author

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

@kellertuer
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

kellertuer commented Jul 1, 2023

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
Copy link
Member Author

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
Copy link
Member Author

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

@kellertuer kellertuer added documentation Improvements or additions to documentation Ready-for-Review A label for pull requests that are feature-ready labels Jul 1, 2023
@mateuszbaran
Copy link
Member

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
Copy link
Member Author

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 kellertuer mentioned this pull request Jul 2, 2023
@kellertuer
Copy link
Member Author

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
Copy link
Member

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

As far as I know, we can't.

@kellertuer
Copy link
Member Author

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

@mateuszbaran
Copy link
Member

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

@mateuszbaran
Copy link
Member

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

@kellertuer
Copy link
Member Author

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

@kellertuer
Copy link
Member Author

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

Co-authored-by: Mateusz Baran <mateuszbaran89@gmail.com>
@kellertuer
Copy link
Member Author

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
Copy link
Member

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

@kellertuer kellertuer merged commit de79731 into master Jul 2, 2023
15 checks passed
@kellertuer kellertuer deleted the kellertuer/introduce-tutorials branch May 4, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation Ready-for-Review A label for pull requests that are feature-ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants