Improve ExtractAndAppendVariablesFromExpression.

[hongkai-dai]

Use std::vectorsymbolic::Variable instead of Eigen::VectorXsymbolic::Variable for faster resizing.

---

This change is 

[hongkai-dai]

+@jwnimmer-tri for feature review please, thanks!

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @hongkai-dai)

[jwnimmer-tri]

CI shows linting failures.

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @hongkai-dai)

---

common/symbolic/test/decompose_test.cc line 233 at r1 (raw file):

GTEST_TEST(SymbolicExtraction, ExtractAndAppendVariablesFromExpression) {
  // Test ExtractAndAppendVariablesFromExpression

nit This comment is pointless (and lacking punctuation). Obviously this is a test case for that function, as can be clearly seen in name of the GTEST_TEST on the prior line.

---

common/symbolic/test/decompose_test.cc line 238 at r1 (raw file):

  Expression e = x + y;
  VectorX<Variable> vars_expected(2);
  vars_expected << x, y;

The vars_expected is never used for anything.

---

common/symbolic/test/decompose_test.cc line 243 at r1 (raw file):

  MapVarToIndex map_var_to_index;
  ExtractAndAppendVariablesFromExpression(e, &vars, &map_var_to_index);
  for (const auto& var : {x, y}) {

Without EXPECT_EQ(map_var_to_index.size(), 2); here (or similar), this test case will fail to notice certain kinds of bugs.

---

common/symbolic/test/decompose_test.cc line 251 at r1 (raw file):

  const Variable z("z");
  e += x * (z - y);

Because this second expression passed to the function under test contains a superset of variables from the first expression we tested above, this test case lacks the power to determine whether or not the function under test appends to the arguments or clears and repopulates the arguments from scratch. The first expression should have variables which are not in the second expression, and we should check that the vector and map retain the extra variable(s) from the first time around.

---

common/symbolic/decompose.h line 47 at r1 (raw file):

    EigenPtr<Eigen::MatrixXd> M, EigenPtr<Eigen::VectorXd> v);

/** Given an expression `e`, extract all variables inside `e`, append these

nit Wrong GSG verb in API doc; should be "extracts" and "appends".

---

common/symbolic/decompose.h line 57 at r1 (raw file):

`vars`, and map_var_to_index[vars(i).get_id()] = i. This invariance holds for
map_var_to_index both as the input and as the output.
@note This function is very slow if you call this function within a loop as it

Is this @note still accurate?

---

common/symbolic/decompose.cc line 188 at r1 (raw file):

void ExtractAndAppendVariablesFromExpression(
    const Expression& e, std::vector<Variable>* vars,
    std::unordered_map<Variable::Id, int>* map_var_to_index) {

nit Missing safety checks for DRAKE_THROW_UNLESS(____ != nullptr); on vars and map_var_to_index.

---

common/symbolic/decompose.cc line 190 at r1 (raw file):

    std::unordered_map<Variable::Id, int>* map_var_to_index) {
  DRAKE_DEMAND(static_cast<int>(map_var_to_index->size()) ==
               static_cast<int>(vars->size()));

nit Do not abort the entire program when the user passes wrong inputs. Use DRAKE_THROW_UNLESS here, not DRAKE_DEMAND.

---

common/symbolic/decompose.cc line 190 at r1 (raw file):

    std::unordered_map<Variable::Id, int>* map_var_to_index) {
  DRAKE_DEMAND(static_cast<int>(map_var_to_index->size()) ==
               static_cast<int>(vars->size()));

nit The static_casts here are pointless. The sizes are the same type and so can be directly compared, without casting.

---

common/symbolic/decompose.cc line 192 at r1 (raw file):

               static_cast<int>(vars->size()));
  for (const Variable& var : e.GetVariables()) {
    if (map_var_to_index->find(var.get_id()) == map_var_to_index->end()) {

BTW This could use try_emplace instead of find, which might be simpler to reason about. The return value indicates whether or not we need to push_back the var.

[hongkai-dai]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

common/symbolic/test/decompose_test.cc line 243 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Without EXPECT_EQ(map_var_to_index.size(), 2); here (or similar), this test case will fail to notice certain kinds of bugs.

Done. Good call.

---

common/symbolic/test/decompose_test.cc line 251 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Because this second expression passed to the function under test contains a superset of variables from the first expression we tested above, this test case lacks the power to determine whether or not the function under test appends to the arguments or clears and repopulates the arguments from scratch. The first expression should have variables which are not in the second expression, and we should check that the vector and map retain the extra variable(s) from the first time around.

Done. Good call.

[jwnimmer-tri]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

[jwnimmer-tri]

Reviewable status: needs at least two assigned reviewers