fix(lcs-factory-options) : breaking python tests using plant for number of contacts resolution

[Meow404]

This change is 

[github-actions]

LCOV of commit e34bbc9 during C3 Coverage #144

Summary coverage rate:
  lines......: 92.2% (2078 of 2253 lines)
  functions..: 80.3% (192 of 239 functions)
  branches...: 49.0% (1696 of 3462 branches)

Files changed coverage rate:
                                                          |Lines       |Functions  |Branches    
  Filename                                                |Rate     Num|Rate    Num|Rate     Num
  ==============================================================================================
  multibody/lcs_factory.cc                                | 4.7%    359|3671%    17|    -      0
  multibody/lcs_factory.h                                 |33.3%      6| 600%     2|    -      0
  systems/c3_controller.cc                                | 4.2%    143|3833%     6|    -      0
  systems/lcs_factory_system.cc                           |15.2%     33|1450%     4|    -      0

[xuanhien070594]

multibody/lcs_factory.cc line 820 at r1 (raw file):

int LCSFactory::GetNumContactVariables(
    const LCSFactoryOptions& options,
    const drake::multibody::MultibodyPlant<double>* plant) {

What if the users use different plant than the one passed to LCSFactory constructor?
I think we can create an utility function that is separate from LCSFactory class.
Also, do we have way to cache the computed values (expanded configs, n_contacts) as we don't want to recompute them?

[xuanhien070594]

I have only one comment. Otherwise, LGTM.

@xuanhien070594 reviewed 19 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Meow404).

[Meow404]

@Meow404 made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on xuanhien070594).

---

multibody/lcs_factory.cc line 820 at r1 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

What if the users use different plant than the one passed to LCSFactory constructor?
I think we can create an utility function that is separate from LCSFactory class.
Also, do we have way to cache the computed values (expanded configs, n_contacts) as we don't want to recompute them?

1. right now there isn't anything preventing the user to using a different plant. Isnt this desirable?
2. Does'nt LCSFactory determine how many lamnda's will be present in the LCS?
3. I had tried working on caching, the issue is then I realized I need a mechanism to check if variables use to calculate the values may have changed over time. Not sure if this added overhead is worth the change. eg. for num_of_friction_direction, once the user calls ResolveNumFrictionDirections and we cache the value, then we need to monitor contact_model, num\_friction\_directions\_per\_contact and num\_friction\_directions for changes.

[xuanhien070594]

multibody/lcs_factory.cc line 820 at r1 (raw file):

Previously, Meow404 (Thomas Stephen Felix) wrote…

1. right now there isn't anything preventing the user to using a different plant. Isnt this desirable?
2. Does'nt LCSFactory determine how many lamnda's will be present in the LCS?
3. I had tried working on caching, the issue is then I realized I need a mechanism to check if variables use to calculate the values may have changed over time. Not sure if this added overhead is worth the change. eg. for num_of_friction_direction, once the user calls ResolveNumFrictionDirections and we cache the value, then we need to monitor contact_model, num\_friction\_directions\_per\_contact and num\_friction\_directions for changes.

That's fair, but if the users pass a plant that is different from the one in the constructor without noticing, the results might cause confusion.
I understand that we want to keep this function as static, so users can use without constructing LCSFactory object, so do you think we should provide another function that use internal options and plant

Code snippet:

static int ComputeNumContactVariables(
    const LCSFactoryOptions& options,
    const MultibodyPlant<double>* plant);

int LCSFactory::GetNumContactVariables() const {
  return ComputeNumContactVariable(options_, plant_);
}

[Meow404]

@Meow404 made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on xuanhien070594).

---

multibody/lcs_factory.cc line 820 at r1 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

That's fair, but if the users pass a plant that is different from the one in the constructor without noticing, the results might cause confusion.
I understand that we want to keep this function as static, so users can use without constructing LCSFactory object, so do you think we should provide another function that use internal options and plant

Done.

[xuanhien070594]

@xuanhien070594 reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on Meow404).