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

FixUp GCS PhiConstraint test #21330

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

RussTedrake
Copy link
Contributor

@RussTedrake RussTedrake commented Apr 19, 2024

PR #21053 to upgrade clarabel fell over on some numerical problems in the PhiConstraint unit test. I've captured that failure and reported it in oxfordcontrol/Clarabel.rs#96 .

In examining the problem, I found a number of small errors in the existing test. Resolving those errors allows clarabel 7.1 to pass the tests without any upstream changes required.

The changes in the PhiConstraint test are:

  • QuadraticCost was being used incorrectly (we forgot the factor of 2). It was also documented as being the L2 norm, instead of the L2 norm squared.

  • Added a diagram to the doc and minor code cleanup.

In GCS, I made two changes, both of which have no functional impact:

  • The bounding box constraint with lb==ub was replaced with a linear equality constraint (which should be preferred).

  • I've slightly improved the variable names used in the mathematical program, for readability during debugging. This program is intentionally not exposed to the user, and all of the changes were to internally defined variables. Although a user could have gotten to these by mucking around in the MathematicalProgramResult, there is nothing in the API contract that would make this a breaking change.

+@bernhardpg for feature review, please.


This change is Reviewable

PR RobotLocomotion#21053 to upgrade clarabel fell over on some numerical problems in
the PhiConstraint unit test.  I've captured that failure and reported
it in oxfordcontrol/Clarabel.rs#96 .

In examining the problem, I found a number of small errors in the
existing test. Resolving those errors allows clarabel 7.1 to pass the
tests without any upstream changes required.

The changes in the PhiConstraint test are:

- QuadraticCost was being used incorrectly (we forgot the factor of
  2). It was also documented as being the L2 norm, instead of the L2
  norm squared.

- Added a diagram to the doc and minor code cleanup.

In GCS, I made two changes, both of which have no functional impact:

- The bounding box constraint with lb==ub was replaced with a linear
  equality constraint (which should be preferred).

- I've slightly improved the variable names used in the mathematical
  program, for readability during debugging. This program is
  intentionally not exposed to the user, and all of the changes were
  to internally defined variables. Although a user _could_ have gotten
  to these by mucking around in the MathematicalProgramResult, there
  is nothing in the API contract that is changing on them.
@RussTedrake RussTedrake added release notes: fix This pull request contains fixes (no new features) release notes: none This pull request should not be mentioned in the release notes priority: medium and removed release notes: fix This pull request contains fixes (no new features) labels Apr 19, 2024
Copy link
Contributor

@AlexandreAmice AlexandreAmice left a comment

Choose a reason for hiding this comment

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

-@bernhardpg +@AlexandreAmice for feature review.

Reviewable status: LGTM missing from assignee AlexandreAmice, needs platform reviewer assigned, needs at least two assigned reviewers

@jwnimmer-tri

This comment was marked as resolved.

@jwnimmer-tri jwnimmer-tri self-assigned this Apr 19, 2024
Copy link
Contributor

@AlexandreAmice AlexandreAmice left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform)


geometry/optimization/test/graph_of_convex_sets_test.cc line 1404 at r1 (raw file):

//   │        ┌──┐      │
//   └───────►│v1├──────┘
//            └──┘

BTW: is this rendering correctly in your IDE? I know that reviewable sometimes renders ascii art strangely.

Code quote:

//            ┌──┐
//   ┌───────►│v2├──────┐
//   │        └──┘      │
//   │                  ▼
// ┌─┴┐                ┌──┐
// │v0│                │v3│
// └─┬┘                └──┘
//   │                  ▲
//   │        ┌──┐      │
//   └───────►│v1├──────┘
//            └──┘

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

:lgtm: platform.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),AlexandreAmice


geometry/optimization/test/graph_of_convex_sets_test.cc line 1404 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

BTW: is this rendering correctly in your IDE? I know that reviewable sometimes renders ascii art strangely.

It displays fairly well for me in reviewable. Locally my font doesn't quite get the monospace right, but the same is true for other ASCII art figures in other tests in this file.

@jwnimmer-tri jwnimmer-tri merged commit 1b01bab into RobotLocomotion:master Apr 19, 2024
10 checks passed
@RussTedrake RussTedrake deleted the gcs_phi_constraint branch June 29, 2024 20:29
RussTedrake added a commit to RussTedrake/drake that referenced this pull request Dec 15, 2024
PR 21053 to upgrade clarabel fell over on some numerical problems in
the PhiConstraint unit test.  I've captured that failure and reported
it in oxfordcontrol/Clarabel.rs#96 .

In examining the problem, I found a number of small errors in the
existing test. Resolving those errors allows clarabel 7.1 to pass the
tests without any upstream changes required.

The changes in the PhiConstraint test are:

- QuadraticCost was being used incorrectly (we forgot the factor of
  2). It was also documented as being the L2 norm, instead of the L2
  norm squared.

- Added a diagram to the doc and minor code cleanup.

In GCS, I made two changes, both of which have no functional impact:

- The bounding box constraint with lb==ub was replaced with a linear
  equality constraint (which should be preferred).

- I've slightly improved the variable names used in the mathematical
  program, for readability during debugging. This program is
  intentionally not exposed to the user, and all of the changes were
  to internally defined variables. Although a user _could_ have gotten
  to these by mucking around in the MathematicalProgramResult, there
  is nothing in the API contract that is changing on them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: none This pull request should not be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants