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

Hyperrectangle #720

Merged
merged 14 commits into from
Apr 22, 2024
Merged

Hyperrectangle #720

merged 14 commits into from
Apr 22, 2024

Conversation

mateuszbaran
Copy link
Member

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.

@mateuszbaran mateuszbaran added new manifold This issue proposes to implement a new manifold preview docs Add this label if you want to see a PR-preview of the documentation labels Apr 3, 2024
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.58%. Comparing base (746ff52) to head (e785dbf).
Report is 8 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master     #720    +/-   ##
========================================
  Coverage   99.57%   99.58%            
========================================
  Files         113      114     +1     
  Lines       10986    11229   +243     
========================================
+ Hits        10939    11182   +243     
  Misses         47       47            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mateuszbaran
Copy link
Member Author

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.

@mateuszbaran
Copy link
Member Author

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 ?

@mateuszbaran mateuszbaran added the Ready-for-Review A label for pull requests that are feature-ready label Apr 17, 2024
@kellertuer
Copy link
Member

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.

@mateuszbaran
Copy link
Member Author

OK, defining it to be 0 will fix the ambiguity. I can try to explain if you have any specific questions 🙂

@kellertuer
Copy link
Member

I think my main problems are twofold

  • what is this manifold useful for? A bit of documentation before the actual types would be good
  • why does exp potentially lead to points outside the manifold. Is that considered in any 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?

@mateuszbaran
Copy link
Member Author

  • 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.

@mateuszbaran
Copy link
Member Author

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.

@kellertuer
Copy link
Member

I never considered Probability simplex with euclidean metric a serious candidate for a manifold ;)

@mateuszbaran
Copy link
Member Author

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 🙂 .

@mateuszbaran
Copy link
Member Author

Or, more precisely, those are manifolds with corners: https://arxiv.org/abs/0910.3518

@kellertuer
Copy link
Member

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.

@mateuszbaran
Copy link
Member Author

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 🙂 . 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.

Copy link
Member

@kellertuer kellertuer left a comment

Choose a reason for hiding this comment

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

I think all discussed points are clarified. I am looking forward to seeing this “in action”.

@mateuszbaran
Copy link
Member Author

Great. I will sketch something soon-ish.

@mateuszbaran mateuszbaran merged commit 32744d6 into master Apr 22, 2024
26 checks passed
@kellertuer kellertuer deleted the mbaran/orthotope branch May 4, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new manifold This issue proposes to implement a new manifold preview docs Add this label if you want to see a PR-preview of the 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