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

📚 #534

Merged
merged 49 commits into from
Oct 31, 2022
Merged

📚 #534

merged 49 commits into from
Oct 31, 2022

Conversation

kellertuer
Copy link
Member

This starts a tutorial section, for no just with a dummy notebook, because I am still pondering about a good start.

Maybe something like generating data on the sphere and computing a mean and a second short part (or even notebook) on comparing different metric on SPDs?

One could also adapt the Benchmark and illustrate the use of different array types with that.

@kellertuer kellertuer marked this pull request as draft September 23, 2022 07:24
docs/make.jl Outdated Show resolved Hide resolved
docs/make.jl Outdated Show resolved Hide resolved
tutorials/primer.jl Outdated Show resolved Hide resolved
tutorials/primer.jl Outdated Show resolved Hide resolved
tutorials/primer.jl Outdated Show resolved Hide resolved
tutorials/primer.jl Outdated Show resolved Hide resolved
tutorials/primer.jl Outdated Show resolved Hide resolved
tutorials/primer.jl Outdated Show resolved Hide resolved
@kellertuer kellertuer changed the title Starts a tutorial section 📚 Sep 23, 2022
@codecov
Copy link

codecov bot commented Sep 23, 2022

Codecov Report

Merging #534 (f28187a) into master (3bb6a94) will not change coverage.
The diff coverage is 100.00%.

❗ Current head f28187a differs from pull request most recent head 71f7f5d. Consider uploading reports for the commit 71f7f5d to get more accurate results

@@           Coverage Diff           @@
##           master     #534   +/-   ##
=======================================
  Coverage   98.93%   98.93%           
=======================================
  Files         100      100           
  Lines        9664     9664           
=======================================
  Hits         9561     9561           
  Misses        103      103           
Impacted Files Coverage Δ
src/Manifolds.jl 100.00% <ø> (ø)
src/differentiation/bvp.jl 100.00% <ø> (ø)
src/differentiation/riemannian_diff.jl 95.23% <ø> (ø)
src/groups/connections.jl 100.00% <ø> (ø)
src/manifolds/GeneralUnitaryMatrices.jl 100.00% <ø> (ø)
src/manifolds/GrassmannProjector.jl 100.00% <ø> (ø)
src/manifolds/GrassmannStiefel.jl 100.00% <ø> (ø)
src/manifolds/Sphere.jl 100.00% <ø> (ø)
src/manifolds/Stiefel.jl 99.35% <ø> (ø)
src/manifolds/StiefelSubmersionMetric.jl 97.79% <ø> (ø)
... and 8 more

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

@mateuszbaran
Copy link
Member

Good idea, and also a good PR name 😄

@kellertuer kellertuer added the preview docs Add this label if you want to see a PR-preview of the documentation label Oct 2, 2022
tutorials/getstarted.jl Show resolved Hide resolved
tutorials/getstarted.jl Show resolved Hide resolved
tutorials/getstarted.jl Show resolved Hide resolved
tutorials/getstarted.jl Show resolved Hide resolved
@kellertuer
Copy link
Member Author

@FedeClaudi I started a tutorial, but I am not sure this is too plain to start with, see

https://github.com/JuliaManifolds/Manifolds.jl/blob/kellertuer/start-tutorials/tutorials/getstarted.jl

(can also be opened as a Pluto Notebook and hopefully renders fine also in the docs on the server, it did locally already, but the server is not yet sure).

Let me know what you think and whether this is a nice start (since I am still unsure how and where to start).

@kellertuer
Copy link
Member Author

The Pluto notebook finally renders

https://juliamanifolds.github.io/Manifolds.jl/previews/PR534/tutorials/getstarted.html

@FedeClaudi
Copy link
Contributor

That's great! I really like this slower approach introducing all concepts one at the time. Great job

@kellertuer
Copy link
Member Author

Ok, then I will continue this one, as soon as my teaching (and my language course) allows.

@mateuszbaran
Copy link
Member

I wonder if there would be the same error if there was no using GLMakie at all?

@kellertuer
Copy link
Member Author

I will try but I fear we might need that for the 3D rendering?

@mateuszbaran
Copy link
Member

Yes, we need GLMakie, I'm mostly curious if that's a PlutoStaticHTML.jl bug.

@kellertuer
Copy link
Member Author

kellertuer commented Oct 26, 2022

I am not sure whether its PlutoStaticHTML or even Pluto itself (as well).

edit: It is already Pluto itself.

@kellertuer
Copy link
Member Author

I started a discussion at fonsp/Pluto.jl#2345

@kellertuer
Copy link
Member Author

I think this might be a larger issue, so what do you think about leaving the tutorial in the repository but for now not putting it in the docs?
That way we could finish this PR and include it as soon as there is a solution?

@mateuszbaran
Copy link
Member

I think that's also fine, this issue with Pluto/GLMakie shouldn't block this PR.

@kellertuer
Copy link
Member Author

Ok, so later today I will remove the second tutorial for now and open an issue for that to keep track :)

Copy link
Member

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

A few quick comments 🙂 .

@@ -7,6 +7,16 @@ in
TangentSpace
````

# Fallback for the exponential map: Solving the corresponding ODE

When additionally loading [`NLSolve.jl`](https://github.com/JuliaNLSolvers/NLsolve.jl) the following fallback for the exponential map is available.
Copy link
Member

Choose a reason for hiding this comment

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

NLSolve is for approximate inverse retraction, not the exponential map.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm? I am confused. It is either approximating Log or exp but approximating a retraction would not make sense (also I think I did not change this, but to fix errors is of course always good).

Copy link
Member

Choose a reason for hiding this comment

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

NLSolve is for approximating Log, for Exp we need OrdinaryDiffEq.

Copy link
Member Author

Choose a reason for hiding this comment

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

That (to me) also sounds a little strange to have two dependencies there, but then ist log probably, yes.

tutorials/getstarted.jl Outdated Show resolved Hide resolved
tutorials/getstarted.jl Outdated Show resolved Hide resolved
tutorials/getstarted.jl Outdated Show resolved Hide resolved
tutorials/getstarted.jl Outdated Show resolved Hide resolved
tutorials/getstarted.jl Outdated Show resolved Hide resolved
tutorials/getstarted.jl Outdated Show resolved Hide resolved
tutorials/getstarted.jl Outdated Show resolved Hide resolved
tutorials/getstarted.jl Outdated Show resolved Hide resolved
@kellertuer
Copy link
Member Author

The problem was that ArrayPartition is currently not exported. Should we export that or do you think that name is too generic for that?

For a beginners tutorial I found ProductRepr nicer as a name.

@mateuszbaran
Copy link
Member

Yes, maybe we should export that. When I use ArrayPartition I always just do using RecursiveArrayTools so I forgot about that.
Regarding ProductRepr, I'm currently not sure that it is needed. I'm considering removing it at some point.

@kellertuer
Copy link
Member Author

Ok, will add the export.
Oh I did not say anything about usefulness, just that in the tutorial at that place the name fits better and is easier to understand than ArrayPartition. We could also leave ProductRepr or ProductRepresentation as an alias for ArrayPartition when we remove ProductRepr as its own type?

@mateuszbaran
Copy link
Member

Yes, we could make ProductRepr an alias for ArrayPartition in the future. What I meant regarding usefulness is that we probably shouldn't advertise functionality that is not useful in our tutorial :).

@kellertuer
Copy link
Member Author

That is why I came up with the idea of the alias (or just slight renaming). Sure we should not advertise deprecated things, just as a novel user working with a product manifold, ProductRepr sounds reasonable and ArrayPartition only after some explanation (ah but I could add that explanation as well why we want to partition the array at that point).

@kellertuer
Copy link
Member Author

I added a small remark on the ArrayPartition.

The only problem is that until releasing this version here the Manifold. has to stay upfront, because only after updating the Pluto notebook afterwards, the version used in the notebook is too old.

But besides this, the PR should be finished.

@kellertuer kellertuer added the Ready-for-Review A label for pull requests that are feature-ready label Oct 28, 2022
@mateuszbaran
Copy link
Member

The only problem is that until releasing this version here the Manifold. has to stay upfront, because only after updating the Pluto notebook afterwards, the version used in the notebook is too old.

I think that's fine, we can remove Manifold. in a future update.

@kellertuer kellertuer removed Ready-for-Review A label for pull requests that are feature-ready preview docs Add this label if you want to see a PR-preview of the documentation labels Oct 31, 2022
@kellertuer kellertuer merged commit 5bbedad into master Oct 31, 2022
@kellertuer kellertuer deleted the kellertuer/start-tutorials branch November 1, 2022 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants