JuliaManifolds / ManifoldsBase.jl

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

Introduce fill and fill! on Power manifolds. #190

Closed kellertuer closed 2 months ago

kellertuer commented 2 months ago

This introduces fill(M,p) to generate P = (p,...,p) on the power manifold. as well as an in-place fill!(M, P, p), which I might find helpful in Manopt.jl soon.

We could discuss

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.90%. Comparing base (e760dc5) to head (0ca3cbd).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #190 +/- ## ========================================== - Coverage 99.96% 99.90% -0.07% ========================================== Files 30 30 Lines 3241 3257 +16 ========================================== + Hits 3240 3254 +14 - Misses 1 3 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mateuszbaran commented 2 months ago

I think using repeat rather than fill would be more appropriate here.

  • the current form generates a few ambiguities, but it follows our idea to have M first; one could argue that M is the size argument here and should therefore be the second argument for that reason (would remove the ambiguities as well).

If we add methods to generic fill or repeat, we should follow their convention and put manifold second IMO. Integers are valid points on a manifold. fill(PowerManifold(Circle(), 3), 4) already has an established meaning different from what you propose here.

kellertuer commented 2 months ago

fill(PowerManifold(Circle(), 3), 4) already has an established meaning different from what you propose here.

Oh, I was not aware of that, what is that meaning?

I had fill in mind since I want to fill the power manifold point with the ball p. But Maybe you are right we should then switch the order, since M here is really something very similar to a size/dimension specification.

kellertuer commented 2 months ago

Ah and for repeat, I feel that might be something that sometimes fits, if one has the ArrayRepresentation in mind, but not in general maybe? So I would prefer fill I think.

mateuszbaran commented 2 months ago

Oh, I was not aware of that, what is that meaning?

julia> fill(PowerManifold(Circle(), 3), 4)
4-element Vector{PowerManifold{ℝ, Circle{ℝ}, Tuple{Int64}, ArrayPowerRepresentation}}:
 PowerManifold(Circle(ℝ), 3)
 PowerManifold(Circle(ℝ), 3)
 PowerManifold(Circle(ℝ), 3)
 PowerManifold(Circle(ℝ), 3)

:slightly_smiling_face:

I had fill in mind since I want to fill the power manifold point with the ball p.

You can also thinking about it as repeating point p for every component.

kellertuer commented 2 months ago

Oh, indeed, hehe; I did only think of “definitions of fill within ManifoldsBase.jl”.

We can also use both reapeat and fill, but I fear they actually would do the same basically then?

mateuszbaran commented 2 months ago

We can also use both reapeat and fill, but I fear they actually would do the same basically then?

I prefer repeat but I'm also OK with fill as long as manifold is not the first argument.

mateuszbaran commented 2 months ago

And yes, there is no reason to have both fill and repeat.

kellertuer commented 2 months ago

My feeling is that for fill you need a size idea which the power manifold provides intuitively, while repeat requires a number-of-repetitions, which (at least intuitively for me) the power manifold only a-bit-implicitly provides. So I would prefer fill, also because we can have fill! as well.

kellertuer commented 2 months ago

I prefer repeat but I'm also OK with fill as long as manifold is not the first argument.

I totally agree that here the manifold should be second.

mateuszbaran commented 2 months ago

My feeling is that for fill you need a size idea which the power manifold provides intuitively, while repeat requires a number-of-repetitions, which (at least intuitively for me) the power manifold only a-bit-implicitly provides. So I would prefer fill, also because we can have fill! as well.

OK, let's use fill then.

kellertuer commented 2 months ago

The two lines we lost look strange in parts I did not even change. Besides that we now have fill and fill! with the better order of parameters.

mateuszbaran commented 2 months ago

I've added support array power representation here. I wouldn't worry about those two missed lines. I think it looks more or less fine now but I'd like to clear your question from that comment before accepting.

kellertuer commented 2 months ago

The Nested replacing case still needs a test.

mateuszbaran commented 2 months ago

I've added that test. I'm not sure now if nested non-replacing should perhaps make copies of p?

kellertuer commented 2 months ago

I've added that test. I'm not sure now if nested non-replacing should perhaps make copies of p?

I think it is good as is, replacing has the copy in its name so it copies, nested does not.

mateuszbaran commented 2 months ago

Yes, it works fine indeed, though the other way (replacing doesn't make copies but non-replacing makes them).