Skip to content

Fix DebugFeasibility#544

Merged
kellertuer merged 6 commits intoJuliaManifolds:masterfrom
JoshuaLampert:fix-debugfeasibility
Nov 19, 2025
Merged

Fix DebugFeasibility#544
kellertuer merged 6 commits intoJuliaManifolds:masterfrom
JoshuaLampert:fix-debugfeasibility

Conversation

@JoshuaLampert
Copy link
Copy Markdown
Contributor

This PR fixes two things:

  1. A typo: ineq_pos is not defined. This should be ineqc_pos.
  2. I am not quite sure about the second one. My equality constraint is scalar. So I pass a function returning a scalar to h of the ConstrainedManifoldObjective. I am not sure if this is supposed to be officially supported or if only vector-valued constraints are supported. But since during the optimization there was no error complaining about h returning a scalar I thought it probably is supported. However, in this case DebugFeasibility does not support it because it would index a scalar. Here, I provide a quick fix for that, which works for scalar h, but it is also not particularly clean. What's your take on that?

@kellertuer
Copy link
Copy Markdown
Member

Thanks for fixing the typo,

for the “just one” (in)eq, until now it is expected that that is a 1-element array and not a scalar. The simple reason is, that this makes a lot of code simpler when we have a unified (and mutable) type. I am not sure how far the code works for scalars and what we would have to check as well.

So if, I would prefer to check the docs to clarify that the functions g and h are expected to return vectors.

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (c10dec8) to head (b00657a).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #544   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           90        90           
  Lines         9967      9967           
=========================================
  Hits          9967      9967           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JoshuaLampert
Copy link
Copy Markdown
Contributor Author

Ok I see. I will try to use a one-element vector constraint in this case and revert the changes regardining this (once I am back at my computer).

@kellertuer
Copy link
Copy Markdown
Member

Nice.

One thing we can check is whether we can wrap that accordingly, i.e. that somehow the constrained cost can contain either case and all accessors (get_cost(s), get_gradient(s)) are able to take that into account but deliver consistently the same. Maybe that is also done to some extend but not used in the place you are here. I can check tomorrow.

If that is not yet done, the challenge is, that the range of a function is not something we can directly “see” when we store the function somewhere, so then we have to find a way to handle that somehow efficiently.

@JoshuaLampert
Copy link
Copy Markdown
Contributor Author

Ok, I reverted the change to point 2. Passing a one-element vector for both h and grad_h worked nicely. Even more than that: The NaN issue I reported in #540 is solved (more on that in that issue) 😅

Comment thread src/plans/debug.jl
@JoshuaLampert
Copy link
Copy Markdown
Contributor Author

Oh, one more small thing I just noted: :Feasibility was missing from the docstring of DebugActionFactory.

@kellertuer kellertuer merged commit 7571c84 into JuliaManifolds:master Nov 19, 2025
14 checks passed
@JoshuaLampert JoshuaLampert deleted the fix-debugfeasibility branch November 19, 2025 12:10
kellertuer referenced this pull request Nov 26, 2025
…551)

Updates the requirements on [ManifoldsBase](https://github.com/JuliaManifolds/ManifoldsBase.jl) and [Manopt](https://github.com/JuliaManifolds/Manopt.jl) to permit the latest version.

Updates `ManifoldsBase` to 2.2.1
- [Release notes](https://github.com/JuliaManifolds/ManifoldsBase.jl/releases)
- [Changelog](https://github.com/JuliaManifolds/ManifoldsBase.jl/blob/master/NEWS.md)
- [Commits](JuliaManifolds/ManifoldsBase.jl@v0.1.0...v2.2.1)

Updates `Manopt` to 0.5.28
- [Release notes](https://github.com/JuliaManifolds/Manopt.jl/releases)
- [Changelog](https://github.com/JuliaManifolds/Manopt.jl/blob/master/Changelog.md)
- [Commits](v0.1.0...v0.5.28)

---
updated-dependencies:
- dependency-name: ManifoldsBase
  dependency-version: 2.2.1
  dependency-type: direct:production
  dependency-group: all-julia-packages
- dependency-name: Manopt
  dependency-version: 0.5.28
  dependency-type: direct:production
  dependency-group: all-julia-packages
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

2 participants