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
368 stars 53 forks source link

Extend support for SpecialEuclidean, fiber bundles, optionally static sizes #642

Closed mateuszbaran closed 10 months ago

mateuszbaran commented 1 year ago

One of the things I'm currently working on is rigid body dynamics using SE(n). I intend to add a bunch of missing features and a tutorial in this PR. The tutorial should potentially supersede #366 .

EDIT: TODO:

codecov[bot] commented 1 year ago

Codecov Report

Merging #642 (34fcb0a) into master (b03cabc) will decrease coverage by 4.54%. The diff coverage is 92.55%.

@@            Coverage Diff             @@
##           master     #642      +/-   ##
==========================================
- Coverage   99.22%   94.68%   -4.54%     
==========================================
  Files         106      108       +2     
  Lines       10488    10562      +74     
==========================================
- Hits        10407    10001     -406     
- Misses         81      561     +480     
Files Coverage Δ
ext/ManifoldsRecipesBaseExt.jl 97.61% <ø> (ø)
ext/ManifoldsTestExt/tests_general.jl 94.44% <100.00%> (-1.28%) :arrow_down:
ext/ManifoldsTestExt/tests_group.jl 99.73% <100.00%> (ø)
src/Manifolds.jl 86.66% <100.00%> (ø)
src/atlases.jl 93.58% <100.00%> (-1.29%) :arrow_down:
src/distributions.jl 66.66% <ø> (ø)
src/groups/GroupManifold.jl 100.00% <100.00%> (ø)
src/groups/addition_operation.jl 100.00% <ø> (ø)
src/groups/circle_group.jl 97.14% <100.00%> (-2.86%) :arrow_down:
src/groups/general_linear.jl 99.19% <100.00%> (+0.09%) :arrow_up:
... and 83 more

... and 6 files with indirect coverage changes

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

mateuszbaran commented 1 year ago

The last commit partially resolves #625 (I haven't updated all manifolds yet).

kellertuer commented 1 year ago

Since this one is breaking, could we base that on a breaking release of ManifoldsBase as well? Would that be a good reason to do that?

mateuszbaran commented 1 year ago

Resolving the exp/retract asymmetry would require a breaking release of ManifoldsBase.jl but I want to do things that don't require breaking Base first.

kellertuer commented 1 year ago

Ok, I was just thinking, since we are breaking something here, why not at the same time break ManifoldsBase as well, but that was also just an idea.

mateuszbaran commented 1 year ago

Thanks for comments!

  • get_n is not a nice name and why can't we use get_parameter (or another function that “extracts” the old {N} type parameter)?

We can have something different, I just wanted names somewhat relevant to the manifold being implemented. For example "get n for SE(n)" sounds to me more straightforward than "get the first parameter of SE(n)". Do you have a better name for that?

  • is it correct that several functions for now only work for the type parameter variant, while the field one is the default?

I'm not sure. I've done the core part of this change and left such problems for later :slightly_smiling_face: . I'm generally fine with both only working for the type parameter variant and having an implementation for field parameter that forwards to the type parameter implementation.

kellertuer commented 1 year ago
  • get_n is not a nice name and why can't we use get_parameter (or another function that “extracts” the old {N} type parameter)?

We can have something different, I just wanted names somewhat relevant to the manifold being implemented. For example "get n for SE(n)" sounds to me more straightforward than "get the first parameter of SE(n)". Do you have a better name for that?

But get_n is too generic and requires too much knowledge on the user side what n is. Sure get_parameter is a technical name, but maybe nicer to have a generic name and having less code per manifold? I would prefer just one times this implementation.

  • is it correct that several functions for now only work for the type parameter variant, while the field one is the default?

I'm not sure. I've done the core part of this change and left such problems for later 🙂 . I'm generally fine with both only working for the type parameter variant and having an implementation for field parameter that forwards to the type parameter implementation.

I am not so much a fan of having them only for the type parameter when the field parameter is the default, since a new user might miss functionality that way that actually exists, just not for their (default, and quite technical) manifold type. How would one to the forwards?

mateuszbaran commented 1 year ago

But get_n is too generic and requires too much knowledge on the user side what n is. Sure get_parameter is a technical name, but maybe nicer to have a generic name and having less code per manifold? I would prefer just one times this implementation.

In the context of a specific manifold get_n seems quite clear to me but I can change it to use get_parameter.

I am not so much a fan of having them only for the type parameter when the field parameter is the default, since a new user might miss functionality that way that actually exists, just not for their (default, and quite technical) manifold type. How would one to the forwards?

The simplest forward would be just

project(M::UnitaryMatrices{Tuple{Int},ℍ}, p) = project(UnitaryMatrices(M.size[1]; parameter=:type), p)

and so on for other methods.

kellertuer commented 1 year ago

I would prefer the slightly technical get_parameter variant, yes.

Concerning the forward – doing that for every manifold is a lot of boiler plate code.

mateuszbaran commented 1 year ago

I would prefer the slightly technical get_parameter variant, yes.

OK.

Concerning the forward – doing that for every manifold is a lot of boiler plate code.

It is, that's why I haven't done it yet. I don't see a better way though.

kellertuer commented 1 year ago

The amount of boiler plate needed is – for me – and argument against this doubling / Parameter/field thing.

For now I do not yet see the actual benefit. It is quite a technical detail – so if we do this, it should not be to the disadvantage of the usability, especially for new users. Missing functions would be a disadvantage I think. But I also do not like boiler plate code.

Hm. What can we best do here?

mateuszbaran commented 1 year ago

For now I do not yet see the actual benefit.

The benefit is in the amount of compilation required to get some results. It's particularly important when someone needs for their computation some manifolds with many different sizes. With precompilation it can also significantly reduce TTFX.

It is quite a technical detail – so if we do this, it should not be to the disadvantage of the usability, especially for new users. Missing functions would be a disadvantage I think. But I also do not like boiler plate code.

Hm. What can we best do here?

Maybe the easiest option would be to keep type-parameter as the default. New users would have full functionality by default with no boilerplate, at the cost of larger TTFX when we start doing precompilation.

kellertuer commented 1 year ago

I am not yet 100% convinced with TTFX,

But if we could keep the parameter as default, since it has more methods implemented – we could “advertise” the new variant as the way to reduce TTFX maybe at the cost of that one or another method still needs to be implemented (for best of cases we somehow find a way to not have too much code duplication).

This would definetly reduce the TTFC (time to first confusion).

mateuszbaran commented 1 year ago

I am not yet 100% convinced with TTFX,

I did a quick test. Without precompilation:

julia> ME = Euclidean(10)
Euclidean(10; field = ℝ)

julia> @time distance(ME, rand(ME), rand(ME))
  0.278328 seconds (690.16 k allocations: 44.318 MiB, 99.97% compilation time)
4.622698026584115

With basic precompilation (in the style of https://github.com/SciML/OrdinaryDiffEq.jl/blob/v6.4.2/src/OrdinaryDiffEq.jl#L185-L214 ):

julia> @time distance(ME, rand(ME), rand(ME))
  0.013193 seconds (512 allocations: 25.812 KiB, 99.57% compilation time)
4.114861767664878

And with type-parameter you pay the price of compilation for each size:

julia> ME2 = Euclidean(11; parameter=:type)
Euclidean(11; field = ℝ, parameter = :type)

julia> @time distance(ME2, rand(ME2), rand(ME2))
  0.080713 seconds (116.56 k allocations: 7.768 MiB, 99.88% compilation time)
3.672744122776624

julia> ME3 = Euclidean(12; parameter=:type)
Euclidean(12; field = ℝ, parameter = :type)

julia> @time distance(ME3, rand(ME3), rand(ME3))
  0.080248 seconds (116.53 k allocations: 7.771 MiB, 99.88% compilation time)
6.3871300778390925

julia> @time distance(ME3, rand(ME3), rand(ME3))
  0.000015 seconds (5 allocations: 656 bytes)
4.808907996393856
kellertuer commented 1 year ago

I see the idea you want to get to, but with parameter as type, your times are much smaller ( factor 3?) than for the field one - or is that some randomness?

Also this will only affect users reusing manifolds but also changing the dimension/size of the manifold. To me this seems to also hint to using parameter=:type as a default. We can maybe switch the default when we notice that parameter=:field is well-supported in most manifolds and indeed in some sense beneficial.

But you convinced me, we can surely have both in parallel for now :) Just, then, please keep :type as the default for now, since it is the one where we are more sure things work.

mateuszbaran commented 1 year ago

I see the idea you want to get to, but with parameter as type, your times are much smaller ( factor 3?) than for the field one - or is that some randomness?

No, with field parameter the TTFX is 20x smaller ( 0.278328 seconds vs 0.013193 seconds)

Also this will only affect users reusing manifolds but also changing the dimension/size of the manifold. To me this seems to also hint to using parameter=:type as a default. We can maybe switch the default when we notice that parameter=:field is well-supported in most manifolds and indeed in some sense beneficial.

It will affect anyone whose size isn't precompiled, not just people reusing the same manifold with a different size (see first comment).

But you convinced me, we can surely have both in parallel for now :) Just, then, please keep :type as the default for now, since it is the one where we are more sure things work.

OK, cool, I will make :type default in this version -- it can be easily changed in the next breaking release if we decide so.

kellertuer commented 1 year ago

I see the idea you want to get to, but with parameter as type, your times are much smaller ( factor 3?) than for the field one - or is that some randomness?

No, with field parameter the TTFX is 20x smaller ( 0.278328 seconds vs 0.013193 seconds)

Now I am confused, maybe since you did not provide all details. So the very first call is a field-type manifold and takes 0.27 seconds? It does not have keyword and currently the default is field.

Then you have the precompiled stuff (that we could introduce) which reduces your recompilation by a factor of 20. But that is not the switch from parameter to field but the precompilation?!

The lat 2 ones on the other hand (our good old parameter type friends) take 0.08 seconds, which is 3x faster than the initial one without precompilcation. That is what I referred to, since for now we do not have precompilation. But you are right in the sense that – for field manifolds we could do precompilations.

mateuszbaran commented 1 year ago

Now I am confused, maybe since you did not provide all details. So the very first call is a field-type manifold and takes 0.27 seconds? It does not have keyword and currently the default is field.

Then you have the precompiled stuff (that we could introduce) which reduces your recompilation by a factor of 20. But that is not the switch from parameter to field but the precompilation?!

The lat 2 ones on the other hand (our good old parameter type friends) take 0.08 seconds, which is 3x faster than the initial one without precompilcation. That is what I referred to, since for now we do not have precompilation. But you are right in the sense that – for field manifolds we could do precompilations.

To make it more straightforward: without precompilation the very first call takes 0.27s either way (both field-parameter and type-parameter). Precompilation (for a different size) + type parameter took 0.08s, while precompilation + field parameter took 0.013s.

mateuszbaran commented 1 year ago

Then you have the precompiled stuff (that we could introduce) which reduces your recompilation by a factor of 20. But that is not the switch from parameter to field but the precompilation?!

So the factor of 20 requires both actually: precompilation and switching to field (or precompiling for the exact size user wants).

kellertuer commented 1 year ago

Ok, I now see that the new type has areas where it might be beneficial. It might still be a bit difficult to convey to the user, but we can maybe to a small tutorial somewhen (maybe even in ManifoldsBase).

mateuszbaran commented 1 year ago

Sure, I will add a small tutorial. I could add it in ManifoldsBase in a non-breaking release even.

kellertuer commented 1 year ago

Since I am currently asking so many questions – could you append to the comment here https://github.com/JuliaManifolds/Manifolds.jl/blob/mbaran/special-euclidean-revisited/src/trait_recursion_breaking.jl#L12 a bit? I am unsure what that file does and the comment is more like: “This is here because it is here.”

mateuszbaran commented 1 year ago

Sure, done. If you need a simple explanation of "what do I need to know to modify this file", then unfortunately it requires delving into profiling and some trial and error, and much more text.

kellertuer commented 1 year ago

Hm, I was expecting a bit more detail, but I at least understand, that the code is not there, because otherwise we get errors, but again to enter racing-areas ;)

In the long run a bit more explanation might be nice though. But I was just stumbling into this while working on the manifold features and was curious. I think long-term it is good to have a bit more comments or explanations in such situations.

mateuszbaran commented 11 months ago

@kellertuer I've tried addressing https://github.com/JuliaManifolds/Manifolds.jl/issues/637#issuecomment-1727115435 and it sort of works but ActionDirectionAndSide looks a bit ugly now IMO. Do you have a better idea? It's mainly needed for all those translate and translate_diff methods.

I've also started making a bit of a cleanup of invariant metrics but I think I will leave a proper rework addressing #471 for later. The change here is "don't use invariance traits if you don't want to use the fallbacks", which limits the impact of a future rework. The problem is that these defaults are introduced in a very non-intuitive way and if we ever wanted patterns that work with group manifolds with partial use of these fallbacks and seamless wrapping in additional decorators, that is simply too much complexity for me.

kellertuer commented 11 months ago

In said comments you and Olivier switched opinions more often than Action Directions, so I have no opinion nor overview on that any longer.

Concerning the metric, I would prefer anyways to have the metric interface in ManifoldsBase at some point. Today I noticed also TangentSpaceAtPoint would be neat in there, but there was something that avoided that we moved that to base? However, for metrics that would be another step unrelated to this PR I think.

mateuszbaran commented 11 months ago

In said comments you and Olivier switched opinions more often than Action Directions, so I have no opinion nor overview on that any longer.

Yes, this is confusing topic but I hope this time I've got it right :slightly_smiling_face: . The main remaining questions are:

  1. Do we want translate(M::AbstractManifold, p, q, conv::ActionDirectionAndSide) or translate(M::AbstractManifold, p, q, conv::ActionDirection, ::GroupActionSide)?
  2. Maybe ForwardSide and BackwardSide would be better than LeftSide and RightSide? I'd like it to not be too confusing for new users, if possible. It's quite unfortunate that many sources don't pay too much attention to this distinction.

Concerning the metric, I would prefer anyways to have the metric interface in ManifoldsBase at some point. Today I noticed also TangentSpaceAtPoint would be neat in there, but there was something that avoided that we moved that to base? However, for metrics that would be another step unrelated to this PR I think.

That would be a good idea but it would require iterating upon it IMO. I wanted to make a small change in this PR that I think is necessary because we make a breaking release anyway but if you prefer a single, comprehensive metric rework later I can leave it out of this PR. I can also put TangentSpaceAtPoint in ManifoldsBase.jl (would be a part of 0.15 together with https://github.com/JuliaManifolds/ManifoldsBase.jl/pull/167).

kellertuer commented 11 months ago

Concerning 1) I do not have a preference. Maybe the second is easier to provide a default for the last parameter and that makes it easier to use this way? Concerning 2) here I really have no preference, left/right forward/backward or even hubward/rimward or turnwise/widdershins (actually the four directions to walk on https://en.wikipedia.org/wiki/Discworld),... I have no preference.

Concerning the metric – yes let‘s do that in another PR to keep this one of finite length.

Concerning TangentSpaceAtPoint – yes for example as part of 0.15, but if I remember correctly there was some reason (dependency?) why you did not want to move all FibreGames to ManifoldsBase?

mateuszbaran commented 11 months ago

Concerning 1) I do not have a preference. Maybe the second is easier to provide a default for the last parameter and that makes it easier to use this way? Concerning 2) here I really have no preference, left/right forward/backward or even hubward/rimward or turnwise/widdershins (actually the four directions to walk on https://en.wikipedia.org/wiki/Discworld),... I have no preference.

OK, I will think about it a bit more or maybe ask on Slack or zulip.

Concerning the metric – yes let‘s do that in another PR to keep this one of finite length.

OK.

Concerning TangentSpaceAtPoint – yes for example as part of 0.15, but if I remember correctly there was some reason (dependency?) why you did not want to move all FibreGames to ManifoldsBase?

No, there was no issue with fibres or tangent spaces. ProductManifold and VectorBundle were problematic because it can't work without RecursiveArrayTools.jl. But now we can make RecursiveArrayTools.jl a package extension to ManifoldsBase.jl so they would be doable in ManifoldsBase.jl too.

kellertuer commented 11 months ago

For Manopt I would love to have TangentSpaceAtPoint in ManifoldsBase (maybe even before finishing the rework on Trust Regions), since it allows to have a nice way to define the lifted (=defined on a tangent space) problems for the sub solvers. I will take a look at tangent space then, probably. But for best of cases, all these meta-manifold should be defined in ManifoldsBase, because then everyone using ManifoldsBase defining their own manifold can have the meta ones already for free.

kellertuer commented 11 months ago

One comment on the inial post already – yes please remove the LinearAffineMetric which is deprecated already and should be AffineInvariantMetroc by now :)

kellertuer commented 11 months ago

Oops sorry for removing the === was probably during debugging something.

mateuszbaran commented 10 months ago

I think this is more or less ready now. We can tag ManifoldsBase.jl 0.15 and check coverage here.

kellertuer commented 10 months ago

Nice! Let‘s do that.

kellertuer commented 10 months ago

In the long run this dependency on each other should be removed, maybe even for both, but especially this here should not (weak/extra) depend on Diff.

mateuszbaran commented 10 months ago

Hm... why does quarto activate a separate project instead of using the same one as docs? I think it just makes it impossible to use this unreleased branch for tutorials?

kellertuer commented 10 months ago

That was initially the idea to make the tutorials more safe / reproducible – and independent of the docs environment. What you can do, is to put Manifolds.jl in development mode in the tutorials environment (and remove it on the next PR after this)?

mateuszbaran commented 10 months ago

This is how it resolves:

  Activating project at `~/work/Manifolds.jl/Manifolds.jl/tutorials`
   Installed InlineStrings ─────── v1.4.0
   Installed BoundaryValueDiffEq ─ v5.1.0
   Installed DiffEqBase ────────── v6.133.1
   Installed NonlinearSolve ────── v2.4.0
   Installed LinearSolve ───────── v2.11.1
   Installed SciMLBase ─────────── v2.4.3
   Installed SentinelArrays ────── v1.4.0
   Installed WorkerUtilities ───── v1.6.1
   Installed WeakRefStrings ────── v1.4.2
   Installed DataFrames ────────── v1.6.1
   Installed PooledArrays ──────── v1.4.3
   Installed Arpack_jll ────────── v3.5.1+1
   Installed MultivariateStats ─── v0.10.2
   Installed ManifoldDiff ──────── v0.3.6
   Installed Arpack ────────────── v0.5.4
   Installed FilePathsBase ─────── v0.9.21
   Installed StringManipulation ── v0.3.4
   Installed PrettyTables ──────── v2.2.8
   Installed ManifoldsBase ─────── v0.14.12
   Installed Manifolds ─────────── v0.8.74
   Installed CSV ───────────────── v0.10.11
   Installed InvertedIndices ───── v1.3.0
    Updating `~/work/Manifolds.jl/Manifolds.jl/tutorials/Project.toml`
  [764a87c0] + BoundaryValueDiffEq v5.1.0
  [336ed68f] + CSV v0.10.11
  [a93c6f00] + DataFrames v1.6.1
  [459566f4] + DiffEqCallbacks v2.33.1
  [31c24e10] + Distributions v0.25.102
  [7073ff75] + IJulia v1.24.2
⌃ [1cead3c2] + Manifolds v0.8.74
⌅ [3362f125] + ManifoldsBase v0.14.12
  [6f286f6a] + MultivariateStats v0.10.2
  [1dea7af3] + OrdinaryDiffEq v6.58.0
  [91a5bcdd] + Plots v1.39.0
  [731186ca] + RecursiveArrayTools v2.38.10
  [276daf66] + SpecialFunctions v2.3.1
  [90137ffa] + StaticArrays v1.6.5

And we get an error that looks like tutorial rendering using these old versions:

An error occurred while executing the following cell:
------------------
is_point(M₆, p₃; error=:error)
------------------

MethodError: no method matching isapprox(::Int64, ::Float64; error::Symbol)

Closest candidates are:
  isapprox(::Number, ::Number; atol, rtol, nans, norm) got unsupported keyword argument "error"
   @ Base floatfuncs.jl:304
  isapprox(::Union{StatsBase.PValue, StatsBase.TestStat}, ::Real; kwargs...)
   @ StatsBase ~/.julia/packages/StatsBase/WLz8A/src/statmodels.jl:104
  isapprox(::SemidirectProductGroup, ::Any, ::Any; kwargs...)
   @ Manifolds ~/.julia/packages/Manifolds/TizHt/src/groups/semidirect_product_group.jl:225
  ...

Stacktrace:
  [1] kwerr(::NamedTuple{(:error,), Tuple{Symbol}}, ::Function, ::Int64, ::Float64)
    @ Base ./error.jl:165
  [2] check_point(M::Hyperbolic{2}, p::Vector{Int64}; kwargs::Base.Pairs{Symbol, Symbol, Tuple{Symbol}, NamedTuple{(:error,), Tuple{Symbol}}})
    @ Manifolds ~/.julia/packages/Manifolds/TizHt/src/manifolds/HyperbolicHyperboloid.jl:29
  [3] (::Manifolds.var"#177#180"{Base.Pairs{Symbol, Symbol, Tuple{Symbol}, NamedTuple{(:error,), Tuple{Symbol}}}})(t::Tuple{Int64, Hyperbolic{2}, Vector{Int64}})
    @ Manifolds ./none:0
  [4] iterate
    @ ./generator.jl:47 [inlined]
  [5] collect(itr::Base.Generator{Tuple{Tuple{Int64, Hyperbolic{2}, Vector{Int64}}, Tuple{Int64, Sphere{2, ℝ}, Vector{Int64}}}, Manifolds.var"#177#180"{Base.Pairs{Symbol, Symbol, Tuple{Symbol}, NamedTuple{(:error,), Tuple{Symbol}}}}})
    @ Base ./array.jl:782
  [6] check_point(M::ProductManifold{ℝ, Tuple{Hyperbolic{2}, Sphere{2, ℝ}}}, p::ArrayPartition{Int64, Tuple{Vector{Int64}, Vector{Int64}}}; kwargs::Base.Pairs{Symbol, Symbol, Tuple{Symbol}, NamedTuple{(:error,), Tuple{Symbol}}})
    @ Manifolds ~/.julia/packages/Manifolds/TizHt/src/manifolds/ProductManifold.jl:231
  [7] check_point
    @ ~/.julia/packages/Manifolds/TizHt/src/manifolds/ProductManifold.jl:229 [inlined]
  [8] #is_point#107
    @ ~/.julia/packages/ManifoldsBase/vKGuo/src/ManifoldsBase.jl:669 [inlined]
  [9] is_point
    @ ~/.julia/packages/ManifoldsBase/vKGuo/src/ManifoldsBase.jl:663 [inlined]
 [10] invoke
    @ ./Base.jl:112 [inlined]
 [11] is_point(::ManifoldsBase.EmptyTrait, M::ProductManifold{ℝ, Tuple{Hyperbolic{2}, Sphere{2, ℝ}}}, p::ArrayPartition{Int64, Tuple{Vector{Int64}, Vector{Int64}}}, te::Bool; kwargs::Base.Pairs{Symbol, Symbol, Tuple{Symbol}, NamedTuple{(:error,), Tuple{Symbol}}})
    @ ManifoldsBase ~/.julia/packages/ManifoldsBase/vKGuo/src/nested_trait.jl:346
 [12] #is_point#192
    @ ~/.julia/packages/ManifoldsBase/vKGuo/src/nested_trait.jl:313 [inlined]
 [13] is_point
    @ ~/.julia/packages/ManifoldsBase/vKGuo/src/nested_trait.jl:308 [inlined]
 [14] is_point(_t::ManifoldsBase.TraitList{IsDefaultMetric{ProductMetric}, ManifoldsBase.TraitList{IsMetricManifold, ManifoldsBase.EmptyTrait}}, M::ProductManifold{ℝ, Tuple{Hyperbolic{2}, Sphere{2, ℝ}}}, p::ArrayPartition{Int64, Tuple{Vector{Int64}, Vector{Int64}}}, te::Bool; kwargs::Base.Pairs{Symbol, Symbol, Tuple{Symbol}, NamedTuple{(:error,), Tuple{Symbol}}})
    @ ManifoldsBase ~/.julia/packages/ManifoldsBase/vKGuo/src/nested_trait.jl:313
 [15] is_point
    @ ~/.julia/packages/ManifoldsBase/vKGuo/src/nested_trait.jl:308 [inlined]
 [16] #is_point#191
    @ ~/.julia/packages/ManifoldsBase/vKGuo/src/nested_trait.jl:306 [inlined]
 [17] is_point
    @ ~/.julia/packages/ManifoldsBase/vKGuo/src/nested_trait.jl:305 [inlined]
 [18] top-level scope
    @ In[31]:1
mateuszbaran commented 10 months ago

That was initially the idea to make the tutorials more safe / reproducible – and independent of the docs environment. What you can do, is to put Manifolds.jl in development mode in the tutorials environment (and remove it on the next PR after this)?

But that just makes it very hard to check if new PR introduces a regression affecting a tutorial without merging and tagging it first?

kellertuer commented 10 months ago

Yes it is a tradeoff. We have the choice of failing tutorials early or being able to adapt tutorials early. When designing all this I had the second in mind.

Still, Just similar to the documenters Project toml you can adapt the tutorial Project toml and add a development mode thingy for Manifolds.jl in the tutorials environment as well. Or even for single tutorials (that is what I did in Manopt.jl).

kellertuer commented 10 months ago

...otherwise, feel free to propose a complete rewrite of this approach, I was happy to find a stable reproducing way after a few weeks of working on this. If you see a better way – sure. Go for that. I would still prefer to have 2 environments for tutorials and docs.

mateuszbaran commented 10 months ago

At the moment I'm not even sure what exactly the interactions of CI, Pkg, Conda and quarto are and all I want is to make the docs working. We can revert whatever makes them work in the next PR or discuss an alternative later.

kellertuer commented 10 months ago

At the moment I'm not even sure what exactly the interactions of CI, Pkg, Conda and quarto are and all I want is to make the docs working. We can revert whatever makes them work in the next PR or discuss an alternative later.

Here is a trick to get a single tutorial use the current dev one https://github.com/JuliaManifolds/Manopt.jl/blob/d6acf643e41a399f4a4e678eeceda2a8a54156cd/tutorials/CountAndCache.qmd#L52-L59 – but I am not sure that works for breaking ones.

As I already wrote – I am not saying my tutorial rendering is the very best solution when it comes to breaking version changes – but after quite a few weeks of work that was the version that did work in a reproducible way.

mateuszbaran commented 10 months ago

Thanks for the tip, I will try it.

kellertuer commented 10 months ago

If it does not I can try to fix it locally here, since I have done that once at least (as I said I first did not have that as a case we do not want to do). The trick should be to locally (in the notebook) dev the right packages ;)

mateuszbaran commented 10 months ago

Documentation works now but normal tests seem to run out of memory on CI. I think we can cut some power or product manifold tests to help with that.

kellertuer commented 10 months ago

Sure, I mean some time after the TR rework I want to get back to minimising the tests anyways. So feel free to minimise the tests (as long as code coverage remains).

mateuszbaran commented 10 months ago

Tests are passing again and coverage is better than before :tada:

mateuszbaran commented 10 months ago

We have warnings about group.md being too big, so I've split actions to a separate page. Even after doing that, we still have a warning:

┌ Warning: Generated HTML over size_threshold_warn limit: manifolds/group.md
│     Generated file size: 166621 (bytes)
│     size_threshold_warn: 102400 (bytes)
│     size_threshold:      204800 (bytes)
│     HTML file:           manifolds/group.html

so in the future we may want to split it even further.

kellertuer commented 10 months ago

Yes we should probably split that – or rethink even the idea of doing a separate Lie groups package (i.e. define the manifolds parts here and the group “extensions” to said manifolds over there).