Snopt solve fortran74 #9896

hongkai-dai · 2018-11-01T15:26:03Z

Allow to compile SnoptSolver with fortran interface in snopt 7.4

This change is

hongkai-dai

@jwnimmer-tri When I set --config snopt_fortran to use SNOPT 7.4, one of the test (the unbounded LP) in snopt_solver_test.cc will fail. Is it possible to select which test to run in snopt_solver_test.cc, given different snopt options (snopt_fortran vs snopt_f2c). I think I will add test on multiple threads, which should be turn on when we compile with snopt_fortran, but off when we use snopt_f2c.

Reviewable status: all discussions resolved, platform LGTM missing

jwnimmer-tri · 2018-11-07T18:56:05Z

What do you think about adding a method on class SnoptSolver, that reports capabilities such as this? At least class SnoptSolver { static bool is_threadsafe(); }, could be used to guard the multi-threaded test. The two backend .cc files could report this answer differently.

It's less satisfying to propose class SnoptSolver { static bool is_unbounded_lp_broken(); } but I'd actually go for that.

The underlying theme in both, is that not only do the unit tests want to know about this, but downstream user code might also need to know, for the same reasons.

hongkai-dai · 2018-11-15T13:37:09Z

@jwnimmer-tri I added the two function is_thread_safe and is_unbounded_lp_broken.

Are we still waiting for snopt 7.4 from RobotLocomotion repo? Is it OK that we merge this PR before we get snopt 7.4?

jwnimmer-tri · 2018-11-16T02:07:04Z

Are we still waiting for snopt 7.4 from RobotLocomotion repo?

Yes. I haven't heard any updates since weeks ago.

Is it OK that we merge this PR before we get snopt 7.4?

Yes, I don't think lack of public CI should hold this up any longer. We should at least begin code review now.

hongkai-dai · 2018-11-16T05:49:38Z

+@avalenzu for feature review please, thanks!

avalenzu · 2018-11-16T16:23:38Z

a discussion (no related file):
I'm unable to build using --config snopt_fortran or --config snopt_f2c. The former yields

INFO: Build options have changed, discarding analysis cache.
ERROR: <drake>/solvers/BUILD.bazel:733:1: in deps attribute of cc_library rule //solvers:snopt_solver:
 source file '//solvers:snopt_solver.cc' is misplaced here (expected .ld, .lds or .ldscript). Since this
 rule was created by the macro 'drake_cc_library', the error might have been caused by the macro
 implementation in <cache>/external/drake/tools/skylark/drake_cc.bzl:294:16                                                                                                                                               
ERROR: <drake>/solvers/BUILD.bazel:733:1: in deps attribute of cc_library rule //solvers:snopt_solver:
 source file '//solvers:snopt_solver_common.cc' is misplaced here (expected .ld, .lds or .ldscript).
 Since this rule was created by the macro 'drake_cc_library', the error might have been caused by the
 macro implementation in  <cache>/external/drake/tools/skylark/drake_cc.bzl:294:16

@jwnimmer-tri, am I doing something wrong? Or is this an issue with the PR?

jwnimmer-tri · 2018-11-16T16:25:46Z

I haven't really looked. To confirm -- you're building from a Drake checkout, with no patches other than this PR, not from an Anzu checkout, is that right?

avalenzu · 2018-11-16T16:49:44Z

That's correct.

avalenzu

Review pass done. PTAL.

Reviewed 9 of 9 files at r1.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee avalenzu, platform LGTM missing

solvers/snopt_solver.cc, line 154 at r1 (raw file):

 * @param grad_index The starting index of the gradient of constraint_list(0)
 * in the optimization problem.
 * @param tx the AutoDiffMatrixType that stores the value of the decision

tx doesn't appear in the argument list, and xvec does. Stale doc?

solvers/snopt_solver.cc, line 167 at r1 (raw file):

    int num_constraints = SingleNonlinearConstraintSize(*c);

    int num_v_variables = binding.GetNumElements();

Is num_v_variables supposed to be num_variables?

solvers/snopt_solver.cc, line 369 at r1 (raw file):

    for (int i = 0; i < n; i++) {
      for (int j = 0; j < static_cast<int>(binding.GetNumElements()); ++j) {

BTW, just to check that I'm reading this right, we're assuming that each constraint's Jacobian is dense?

solvers/snopt_solver.cc, line 755 at r1 (raw file):

           &sInf, &miniw, &minrw, snopt_problem_iu, &snopt_problem_leniu,
           snopt_problem_ru, &snopt_problem_lenru, snopt_problem_iw,
           &snopt_problem_leniw, snopt_problem_rw, &snopt_problem_lenrw);

BTW. Wow. That's a heck of a signature.

solvers/test/optimization_examples.cc, line 208 at r1 (raw file):

void NonConvexQPproblem1::CheckSolution() const {
  const auto& x_value = prog_->GetSolution(x_);
  EXPECT_TRUE(CompareMatrices(x_value, x_expected_, 1.001E-6,

This seems like a pretty big jump in the tolerance, although 1.001e-6 is still pretty tight. Do you know why this change is necessary? Could the tolerance just be 1e-6?

jwnimmer-tri · 2018-11-16T18:09:20Z

I'm unable to build ...

Yes, there were lots of merge errors. I've PRd the fixes at hongkai-dai#15. You can pick the last commit from there, if you want to build locally before it's merged.

avalenzu

Reviewable status: 3 unresolved discussions, LGTM missing from assignee avalenzu, platform LGTM missing

a discussion (no related file):

Previously, avalenzu (Andres Valenzuela) wrote…

I'm unable to build using --config snopt_fortran or --config snopt_f2c. The former yields

INFO: Build options have changed, discarding analysis cache.
ERROR: <drake>/solvers/BUILD.bazel:733:1: in deps attribute of cc_library rule //solvers:snopt_solver:
 source file '//solvers:snopt_solver.cc' is misplaced here (expected .ld, .lds or .ldscript). Since this
 rule was created by the macro 'drake_cc_library', the error might have been caused by the macro
 implementation in <cache>/external/drake/tools/skylark/drake_cc.bzl:294:16                                                                                                                                               
ERROR: <drake>/solvers/BUILD.bazel:733:1: in deps attribute of cc_library rule //solvers:snopt_solver:
 source file '//solvers:snopt_solver_common.cc' is misplaced here (expected .ld, .lds or .ldscript).
 Since this rule was created by the macro 'drake_cc_library', the error might have been caused by the
 macro implementation in  <cache>/external/drake/tools/skylark/drake_cc.bzl:294:16

@jwnimmer-tri, am I doing something wrong? Or is this an issue with the PR?

Jeremy's fixes made this work. Thanks, @jwnimmer-tri !

hongkai-dai · 2018-11-17T08:59:27Z

@jwnimmer-tri Thanks for the PR hongkai-dai#15. I rebased this branch on top of master, and applied part of your PR to this PR. I don't think I know how to keep the commit history clean with merge, so I used rebase instead.

@avalenzu Thanks for the comments!

BTW, just to check that I'm reading this right, we're assuming that each constraint's Jacobian is dense?

That is right. We currently don't specify the sparsity pattern in each constraint. If the constraint depends on y, where y is a subset of all the decision variables x, we then suppose that the gradient is dense w.r.t y, but sparse w.r.t the variables in x but not in y.

This seems like a pretty big jump in the tolerance, although 1.001e-6 is still pretty tight. Do you know why this change is necessary? Could the tolerance just be 1e-6?

I forgot why I increased the tolerance, probably I did that when I tried other versions of SNOPT. I reverted back to the previous tolerance 1E-9.

jwnimmer-tri · 2018-11-17T16:59:07Z

... commit history clean ...

What you did is perfect -- squashing my commit into you own is great. I used PR so you could choose whether / when / how to integrate it, not because I think my commit should be separated out.

jwnimmer-tri

I've feature-reviewed the new code here beyond what #9177 already had (basically the changes from cwrap to f90). I've left some discussions below.

In Drake, I locally tested bazel test //solvers/... --test_tag_filters=-mosek,-gurobi in the following configurations.

With SNOPT_PATH=snopt7-4.tar.gz (7.4-1.1):
--config snopt => all tests passed
--config snopt_f2c => all tests passed
--config snopt_fortran => all tests passed

With SNOPT_PATH=git (7.6.1):
--config snopt => all tests passed
--config snopt_f2c => all tests passed
--config snopt_fortran => does not compile; the "summary file" changes in f90 wrapper snopt/snopt-interface@036f2fa were a flag day API break. In a future PR, we should update our snopt_solver.cc to work with the newer 7.6 f90 API as well, so that we can easily continue to test new releases to see if the upstream bugs are ever fixed. (I can have the build system export a #define or similar, with the snopt version.) For now, this is fine.

FYI When we port Anzu to use this PR, we should move its custom tests like SnoptTest.MultiThreadTest into the Drake repo. (I think it's okay and good to leave them out of this PR though.)

Reviewed 6 of 9 files at r1, 2 of 3 files at r2.
Dismissed @avalenzu from 3 discussions.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee avalenzu, platform LGTM missing

solvers/snopt_solver.cc, line 20 at r2 (raw file):

extern "C" {
// NOLINTNEXTLINE(build/include)
#include "snopt_cwrap.h"

I think we aren't using any snopt_cwrap.h functions; this should be #include "snopt.h" instead?

And then, since snopt.h already has the extern "C" declaration built-in, we can remove the extern "C" { + } declaration here.

(And then, if the linters want to sort this line elsewhere, that should be okay -- it doesn't leak any macros or bad symbols, like the other snopt headers do.)

(Note that the BUILD should should stay unchanged and still say :snopt_cwrap -- there is no smaller target available yet; I will tidy that up later.)

solvers/snopt_solver.cc, line 369 at r2 (raw file):

    for (int i = 0; i < n; i++) {
      for (int j = 0; j < static_cast<int>(binding.GetNumElements()); ++j) {
        (*iGfun)[*grad_index] = 1 + *constraint_index + i;  // row order

nit Elsewhere you say "// Fortran is 1-indexed." That might also help clarify here.

solvers/snopt_solver.cc, line 397 at r2 (raw file):

    (*Fupp)[*constraint_index] = 0;
    for (int j = 0; j < binding.evaluator()->M().rows(); ++j) {
      (*iGfun)[*grad_index] = 1 + *constraint_index;

nit Elsewhere you say "// Fortran is 1-indexed." That might also help clarify here.

solvers/snopt_solver.cc, line 514 at r2 (raw file):

    print_file_name = print_file_it->second;
  }
  int snopt_problem_memCalled = 0;

This variable doesn't seem to ever do anything. There is an if-guard that uses it, but it's always zero when it hits that if statement.

solvers/snopt_solver.cc, line 693 at r2 (raw file):

  for (const auto& it : snopt_options_double) {
    int errors;

nit If we are going to ignore errors, either we should have a comment explaining why that's okay, or a TODO comment to improve it later.

Ditto a few lines below also.

solvers/snopt_solver.cc, line 726 at r2 (raw file):

          realloc(snopt_problem_iw, sizeof(int) * snopt_problem_leniw));
      const std::string option = "Total int workspace";
      int errors;

nit If we are going to ignore errors, either we should have a comment explaining why that's okay, or a TODO comment to improve it later.

Ditto a few lines below also.

jwnimmer-tri

Reviewed 1 of 3 files at r2.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee avalenzu, platform LGTM missing

hongkai-dai · 2018-11-19T01:59:05Z

@jwnimmer-tri I have addressed your comments. Could you take a look after Thanksgiving holiday? Thanks!

I still kept //NOLINTNEXTLINE, since the linter complains that I need to include the directory of the header file.

jwnimmer-tri

This PR is ready for platform review.

Reviewed 1 of 1 files at r3.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee avalenzu, platform LGTM missing, commit history needs curation

solvers/snopt_solver.cc, line 397 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Elsewhere you say "// Fortran is 1-indexed." That might also help clarify here.

nit Would a Fortran is 1-indexed. comment apply here (to line 396) as well?

jwnimmer-tri

Reviewed 1 of 1 files at r4.
Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee avalenzu, platform LGTM missing, commit history needs curation

avalenzu

+@EricCousineau-TRI for platform review.

Reviewed 2 of 3 files at r2, 1 of 1 files at r4.
Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM missing, commit history needs curation

EricCousineau-TRI

For CI, at what point do we want to start testing these different configurations? ASAP?

Reviewed 6 of 9 files at r1, 2 of 3 files at r2, 1 of 1 files at r4.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [ericcousineau-tri], commit history needs curation

solvers/BUILD.bazel, line 736 at r4 (raw file):

    name = "snopt_solver",
    srcs = select({
        "//tools:with_snopt": [

nit Can you add a comment saying that this is a duplicate / alias of //tools:with_snopt_f2c for now?

solvers/snopt_solver.h, line 47 at r4 (raw file):

  static bool is_thread_safe();

  /// For some reason, when I use SNOPT 7.4, the solver fails to detect a simple

nit Can you add your name here? "when I (Hongkai) ..."

solvers/snopt_solver.cc, line 90 at r4 (raw file):

  // Converts the `this` pointer into an integer array.
  std::array<int, kIntCount> MakeThisAsInts() {
    std::array<int, kIntCount> result;

nit Can this stuff be simplified to:

ctor(...) ... {
  reinterpret_cast<SnoptUserFunInfo*&>(this_pointer_as_int_array_.data()) = this;
}
auto& GetFrom(...) {
  DRAKE_DEMAND(...);
  return *reinterpret_cast<SnopUserFuncInfo*>(this_pointer_as_int_array_.data());
}

Then you can remove MakeThisAsInts().

EricCousineau-TRI

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [ericcousineau-tri], commit history needs curation

jwnimmer-tri

For CI, at what point do we want to start testing these different configurations?

My proposal is that we only cover one variety in Drake CI (--config snopt, which is currently the f2c flavor). Once this PR merges (and once we add 7.6 Fortran support), we can ask the Drake community to test --config snopt_fortran and then if there are no problems, switch to it as the default --config snopt, leaving --config snopt_f2c around to allow people to opt-out during a transition period. But in all cases, only have CI test --config snopt (and --config everything).

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [ericcousineau-tri], commit history needs curation

solvers/BUILD.bazel, line 736 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Can you add a comment saying that this is a duplicate / alias of //tools:with_snopt_f2c for now?

Hmm, what's the problem you're trying to solve? A comment that had that exact wording would be more confusing than saying nothing. If you actually explain the defect you've identified, Hongkai or I can propose a resolution that makes sense.

EricCousineau-TRI

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri, platform LGTM from [ericcousineau-tri], commit history needs curation

solvers/BUILD.bazel, line 736 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Hmm, what's the problem you're trying to solve? A comment that had that exact wording would be more confusing than saying nothing. If you actually explain the defect you've identified, Hongkai or I can propose a resolution that makes sense.

OK It's a duplicate of the stuff below; would be nice to quickly indicate that it's because it's the default option.

avalenzu

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri, platform LGTM from [ericcousineau-tri], commit history needs curation

jwnimmer-tri

Oops, I'd also forgotten my stamp.

Reviewed 1 of 1 files at r5.
Reviewable status: 1 unresolved discussion, platform LGTM from [ericcousineau-tri, jwnimmer-tri], commit history needs curation

solvers/BUILD.bazel, line 736 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK It's a duplicate of the stuff below; would be nice to quickly indicate that it's because it's the default option.

Done.

jwnimmer-tri

Reviewed 1 of 1 files at r6.
Reviewable status: all discussions resolved, platform LGTM from [ericcousineau-tri, jwnimmer-tri], commit history needs curation

jwnimmer-tri · 2018-11-27T14:38:01Z

I checked over the code again, and I don't see any hazard to macOS builds, so I'm not going to opt-in to those jenkins jobs.

I think this is ready to merge. Since @hongkai-dai is offline today, I will plan to squash and merge myself once CI is happy.

avalenzu

Reviewable status: all discussions resolved, platform LGTM from [ericcousineau-tri, jwnimmer-tri], commit history needs curation

hongkai-dai mentioned this pull request Nov 1, 2018

cleanup / finish snopt c++ wrapper #1667

Closed

hongkai-dai commented Nov 7, 2018

View reviewed changes

hongkai-dai force-pushed the snopt_solve_fortran74 branch from 94d37d9 to f9a3769 Compare November 15, 2018 13:36

hongkai-dai force-pushed the snopt_solve_fortran74 branch from f9a3769 to ea58d00 Compare November 15, 2018 13:57

hongkai-dai assigned avalenzu Nov 16, 2018

avalenzu requested changes Nov 16, 2018

View reviewed changes

avalenzu reviewed Nov 16, 2018

View reviewed changes

hongkai-dai added 2 commits November 17, 2018 15:42

Add snopt_solver to use SNOPT 7.4 fortran interface.

c06e102

Address Andres' comments.

4cf63bb

hongkai-dai force-pushed the snopt_solve_fortran74 branch from ea58d00 to 4cf63bb Compare November 17, 2018 08:58

jwnimmer-tri self-assigned this Nov 17, 2018

jwnimmer-tri requested changes Nov 17, 2018

View reviewed changes

jwnimmer-tri reviewed Nov 17, 2018

View reviewed changes

jwnimmer-tri added the status: curate commits before merging label Nov 17, 2018

Address Jeremy's comments.

c8703ed

jwnimmer-tri approved these changes Nov 19, 2018

View reviewed changes

jwnimmer-tri added the priority: high label Nov 19, 2018

Add documentation that fortran is 1 indexed.

cb0ff6a

jwnimmer-tri reviewed Nov 20, 2018

View reviewed changes

avalenzu assigned EricCousineau-TRI Nov 26, 2018

avalenzu approved these changes Nov 26, 2018

View reviewed changes

EricCousineau-TRI approved these changes Nov 26, 2018

View reviewed changes

jwnimmer-tri reviewed Nov 26, 2018

View reviewed changes

EricCousineau-TRI approved these changes Nov 27, 2018

View reviewed changes

avalenzu approved these changes Nov 27, 2018

View reviewed changes

Improve snopt select-with defaults documentation

0c8f425

jwnimmer-tri approved these changes Nov 27, 2018

View reviewed changes

Remove first person from comment

98d8449

jwnimmer-tri approved these changes Nov 27, 2018

View reviewed changes

avalenzu approved these changes Nov 27, 2018

View reviewed changes

jwnimmer-tri merged commit a34ee3e into RobotLocomotion:master Nov 27, 2018

jwnimmer-tri mentioned this pull request Nov 29, 2018

SnoptSolver uses C snopt_interface instead of f2c interface. #9177

Closed

hongkai-dai deleted the snopt_solve_fortran74 branch December 15, 2018 01:36

jwnimmer-tri mentioned this pull request Jan 3, 2019

Investigate SNOPT 7.5 new interface #7984

Closed

jwnimmer-tri mentioned this pull request Jan 30, 2019

doc: Mention --config snopt_fortran and related #10543

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snopt solve fortran74 #9896

Snopt solve fortran74 #9896

hongkai-dai commented Nov 1, 2018 •

edited by sherm1

Loading

hongkai-dai left a comment

jwnimmer-tri commented Nov 7, 2018

hongkai-dai commented Nov 15, 2018

jwnimmer-tri commented Nov 16, 2018

hongkai-dai commented Nov 16, 2018

avalenzu commented Nov 16, 2018

jwnimmer-tri commented Nov 16, 2018

avalenzu commented Nov 16, 2018

avalenzu left a comment

jwnimmer-tri commented Nov 16, 2018

avalenzu left a comment

hongkai-dai commented Nov 17, 2018

jwnimmer-tri commented Nov 17, 2018

jwnimmer-tri left a comment

jwnimmer-tri left a comment

hongkai-dai commented Nov 19, 2018

jwnimmer-tri left a comment

jwnimmer-tri left a comment

avalenzu left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

jwnimmer-tri left a comment

EricCousineau-TRI left a comment

avalenzu left a comment

jwnimmer-tri left a comment

jwnimmer-tri left a comment

jwnimmer-tri commented Nov 27, 2018

avalenzu left a comment

Snopt solve fortran74 #9896

Snopt solve fortran74 #9896

Conversation

hongkai-dai commented Nov 1, 2018 • edited by sherm1 Loading

hongkai-dai left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented Nov 7, 2018

hongkai-dai commented Nov 15, 2018

jwnimmer-tri commented Nov 16, 2018

hongkai-dai commented Nov 16, 2018

avalenzu commented Nov 16, 2018

jwnimmer-tri commented Nov 16, 2018

avalenzu commented Nov 16, 2018

avalenzu left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented Nov 16, 2018

avalenzu left a comment

Choose a reason for hiding this comment

hongkai-dai commented Nov 17, 2018

jwnimmer-tri commented Nov 17, 2018

jwnimmer-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

hongkai-dai commented Nov 19, 2018

jwnimmer-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

avalenzu left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

avalenzu left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented Nov 27, 2018

avalenzu left a comment

Choose a reason for hiding this comment

hongkai-dai commented Nov 1, 2018 •

edited by sherm1

Loading