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

[solvers] Add missing unit tests for options handling #21939

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Sep 22, 2024

In preparation for #21345.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: medium release notes: none This pull request should not be mentioned in the release notes labels Sep 22, 2024
@jwnimmer-tri jwnimmer-tri force-pushed the solver-options-specific-newtests branch from dfe1a44 to b7e7dfd Compare September 22, 2024 17:05
@jwnimmer-tri
Copy link
Collaborator Author

+@hongkai-dai for feature review, please.

Copy link
Contributor

@hongkai-dai hongkai-dai left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee hongkai-dai, needs platform reviewer assigned, needs at least two assigned reviewers


solvers/test/mosek_solver_test.cc line 300 at r1 (raw file):

                           bas_file);
  solver.Solve(prog, {}, solver_options, &result);
  EXPECT_TRUE(std::filesystem::exists({log_file}));

Should we check the existence of the bas_file?

Copy link
Collaborator Author

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignee hongkai-dai, needs platform reviewer assigned, needs at least two assigned reviewers


solvers/test/mosek_solver_test.cc line 300 at r1 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Should we check the existence of the bas_file?

Yes, I forgot to check that. However, I can't get MOSEK to write to that file, or any other string-option file for that matter.

Can you think of any https://docs.mosek.com/10.1/capi/parameters.html#doc-sparam-list options that we could set and see some effect from, either in a file or a change to the solution result?

@jwnimmer-tri jwnimmer-tri force-pushed the solver-options-specific-newtests branch from b7e7dfd to d81520a Compare September 25, 2024 21:16
Copy link
Collaborator Author

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

Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee hongkai-dai, needs platform reviewer assigned, needs at least two assigned reviewers


solvers/test/mosek_solver_test.cc line 300 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Yes, I forgot to check that. However, I can't get MOSEK to write to that file, or any other string-option file for that matter.

Can you think of any https://docs.mosek.com/10.1/capi/parameters.html#doc-sparam-list options that we could set and see some effect from, either in a file or a change to the solution result?

I tried a half dozen string options, and none of them wrote anything. AA couldn't think of an option to test, either. So instead, I've rewritten the test code to clarify that all we are doing is smoke testing lack of crashes, on purpose.

@jwnimmer-tri
Copy link
Collaborator Author

+@sammy-tri for platform review per schedule, please.

Copy link
Contributor

@hongkai-dai hongkai-dai 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 1 of 1 files at r2.
Reviewable status: LGTM missing from assignee sammy-tri(platform)


solvers/test/mosek_solver_test.cc line 300 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I tried a half dozen string options, and none of them wrote anything. AA couldn't think of an option to test, either. So instead, I've rewritten the test code to clarify that all we are doing is smoke testing lack of crashes, on purpose.

Sorry I somehow missed this PR. Thanks for the change.

Copy link
Contributor

@sammy-tri sammy-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:

Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),hongkai-dai

@sammy-tri sammy-tri merged commit c29f775 into RobotLocomotion:master Sep 26, 2024
9 checks passed
@jwnimmer-tri jwnimmer-tri deleted the solver-options-specific-newtests branch September 26, 2024 19:54
RussTedrake pushed a commit to RussTedrake/drake that referenced this pull request Dec 15, 2024
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.

3 participants