Closed mateuszbaran closed 4 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 99.58%. Comparing base (
746ff52
) to head (e785dbf
). Report is 8 commits behind head on master.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Regarding activity analysis: https://web.mit.edu/dimitrib/www/ProjectedNewton.pdf . A small modification to direction update rule would be needed so that update is diagonal in directions orthogonal to boundary.
We have an ambiguity here:
injectivity_radius(M::AbstractManifold, m::AbstractRetractionMethod) @ ManifoldsBase ~/.julia/packages/ManifoldsBase/Sk7lI/src/ManifoldsBase.jl:528
injectivity_radius(M::Hyperrectangle, p) @ Manifolds ~/work/Manifolds.jl/Manifolds.jl/src/manifolds/Hyperrectangle.jl:173
I thought I would leave the global injectivity_radius
undefined (it's just 0 and not useful) but seeing this I'm not sure. What do you think @kellertuer ?
I mean you could still define the global one to be zero if it is the case?
For the ambiguity, I would have to carefully check (maybe not this week, last week of lectures, drowning a bit in emails);
Overall I have not yet understood this manifold, I think.
OK, defining it to be 0 will fix the ambiguity. I can try to explain if you have any specific questions :slightly_smiling_face:
I think my main problems are twofold
Besides that I think the inner product might not be correctly documented at first sight. For i=2 X^TY is still a matrix not a number – maybe there is a trace or so missing for this case?
- what is this manifold useful for? A bit of documentation before the actual types would be good
Mostly optimization with box constraints, at least that's my idea.
- why does exp potentially lead to points outside the manifold. Is that considered in any way?
I think we can't assume exponential map is always well-defined for arbitrary vectors. Note that the default retraction is ProjectionRetraction
, and ProbabilitySimplex
with EuclideanMetric
behaves the same way.
Besides that I think the inner product might not be correctly documented at first sight. For i=2 X^TY is still a matrix not a number – maybe there is a trace or so missing for this case?
Yes, it should have a trace. I copied it from Euclidean
which has the same issue.
I never considered Probability simplex with euclidean metric a serious candidate for a manifold ;)
I never considered Probability simplex with euclidean metric a serious candidate for a manifold ;)
It surely is not, it's a manifold with boundary. A different thing :slightly_smiling_face: .
Or, more precisely, those are manifolds with corners: https://arxiv.org/abs/0910.3518
Ok, while I do not see much how this manifold is useful – I think I also do not have to find it useful. I am still a bit confused that retractions do map into the manifold and exp does not, but ok.
The only think I would like to see a bit more proimentnly – maybe in the docs before the list of doc strings starts – would be the note that this is a manifold with boundary, and their interface might not yet bet 100% fixed.
Ok, while I do not see much how this manifold is useful – I think I also do not have to find it useful.
With L-BFGS-B being one of the most widely used optimization algorithm, and working exactly on a hyperrectangle manifold, I think utility should be fairly clear :slightly_smiling_face: . Manopt's qN requires some small tweaks to ensure reasonable convergence for manifolds with boundary or corners but I'm planning on adding that soon-ish.
The only think I would like to see a bit more proimentnly – maybe in the docs before the list of doc strings starts – would be the note that this is a manifold with boundary, and their interface might not yet bet 100% fixed.
Sure, I will add a note. And the interface would definitely have to be extended a bit but I haven't designed that yet in detail.
I am still a bit confused that retractions do map into the manifold and exp does not, but ok.
Locally they both map into the manifold. I'm not sure how to explain that idea clearly but for standard Riemannian manifolds you also may have tangent vectors for which exp
is undefined. Not all manifolds are geodesically complete. It's just that we focus in Manifolds.jl on symmetric manifolds and they are geodesically complete.
Great. I will sketch something soon-ish.
Mostly to make Manopt work with box constraints. The trick is just projecting gradients and projection retraction but it's still better than nothing IMO. Activity analysis needs to be done on Manopt's side.