analysis: Add simulator_flags helper

[jwnimmer-tri]

Closes #12873. Relates #12903.

This extends pydrake.systems.Simulator.reset_integrator's deprecation window because up to this point there was no viable replacement.

---

This change is 

[jwnimmer-tri]

+@amcastro-tri would you like to review the C++ parts of this?

[jwnimmer-tri]

\CC @antequ FYI

[amcastro-tri]

I can't believe you went through all that trouble go generate names from the actual classes. But I love it, learned a few things.

Reviewed 8 of 8 files at r1.
Reviewable status: 4 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @jwnimmer-tri)

---

systems/analysis/simulator.h, line 589 at r1 (raw file):

  template <class U>
  DRAKE_DEPRECATED(
      "2020-08-01",

is this intentional?

---

systems/analysis/simulator_flags.h, line 43 at r1 (raw file):

@ingroup simulator_configuration */
const std::vector<std::string>& GetIntegratorSchemes();

nit, maybe GetIntegrationSchemes() for consistency with gflags.

---

systems/analysis/simulator_flags.cc, line 52 at r1 (raw file):

  // Get the class name, e.g., FooBarIntegrator<double>.
  string full_name = NiceTypeName::Get<Integrator<double>>();
  string class_name = NiceTypeName::RemoveNamespaces(full_name);

nit, const string.

---

systems/analysis/simulator_flags.cc, line 110 at r1 (raw file):

// here must be kept in sync with the help string in simulator_gflags.cc.
template <typename T>
const vector<NamedResetIntegratorFunc<T>>& GetSupportedIntegrators() {

nit, maybe name something like GetSupportedNamedResetIntegratorFunctions()? since it doesn't return integrators but a named function to reset the integrator.

---

systems/analysis/simulator_flags.cc, line 138 at r1 (raw file):

  const auto& name_func_pairs = GetSupportedIntegrators<T>();
  for (const auto& [one_name, one_func] : name_func_pairs) {

btw, beautiful range iteration! I had no idea this syntax was possible.

---

systems/analysis/test/simulator_flags_test.cc, line 18 at r1 (raw file):

  const void* prior_integrator = &simulator.get_integrator();
  IntegratorBase<double>& result = ResetIntegratorFromFlags(
      &simulator, "runge_kutta2", 0.001);

btw, maybe here you could range loop over GetIntegratorSchemes() and verify the right integrator type is created?

[amcastro-tri]

+@sherm1 for platform please.

Reviewable status: 4 unresolved discussions, LGTM missing from assignee sherm1(platform) (waiting on @jwnimmer-tri and @sherm1)

[antequ]

Wow, thanks for doing this!

Reviewable status: 4 unresolved discussions, LGTM missing from assignee sherm1(platform) (waiting on @jwnimmer-tri and @sherm1)

[jwnimmer-tri]

Reviewable status: 4 unresolved discussions, LGTM missing from assignee sherm1(platform) (waiting on @amcastro-tri, @jwnimmer-tri, and @sherm1)

---

systems/analysis/simulator.h, line 589 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

is this intentional?

Yes. I think a good goal is to have a three month overlap between an API being deprecated and a replacement API being available. Since it is only as of this PR that python offers a replacement API for #12520, I think we should retain the old API for another three months. It's easiest to retain it in python if we also keep the C++ backing method unchanged.

---

systems/analysis/simulator_flags.cc, line 138 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

btw, beautiful range iteration! I had no idea this syntax was possible.

https://en.cppreference.com/w/cpp/language/structured_binding

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform) (waiting on @amcastro-tri and @sherm1)

---

systems/analysis/simulator_flags.h, line 43 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

nit, maybe GetIntegrationSchemes() for consistency with gflags.

Done.

---

systems/analysis/simulator_flags.cc, line 110 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

nit, maybe name something like GetSupportedNamedResetIntegratorFunctions()? since it doesn't return integrators but a named function to reset the integrator.

Done.

---

systems/analysis/test/simulator_flags_test.cc, line 18 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

btw, maybe here you could range loop over GetIntegratorSchemes() and verify the right integrator type is created?

Done.

[sherm1]

Neat! Platform pending a few minor things.

Reviewed 3 of 8 files at r1, 5 of 5 files at r2.
Dismissed @amcastro-tri from a discussion.
Reviewable status: 4 unresolved discussions (waiting on @jwnimmer-tri)

---

systems/analysis/simulator_flags.h, line 30 at r2 (raw file):

@param[in] max_step_size The IntegratorBase::set_maximum_step_size() value.
@returns A reference to the the newly created integrator owned by `simulator`.

minor: please add a note that this will only work for integrators whose names follow the informal convention we've been using: BlahIntegrator<T>. Other integrators can be derived from IntegratorBase and we don't currently require any particular naming scheme. Also consider adding a note on IntegratorBase advising a naming convention that will work here.

---

systems/analysis/simulator_flags.h, line 37 at r2 (raw file):

    Simulator<T>* simulator,
    const std::string& scheme,
    const T& max_step_size);

minor: do you have any particular reason for putting the output parameter (really in/out) first here? If not, better to put it after the input parameters per style guide.

---

systems/analysis/simulator_flags.cc, line 51 at r2 (raw file):

string GetIntegratorName() {
  // Get the class name, e.g., FooBarIntegrator<double>.
  string full_name = NiceTypeName::Get<Integrator<double>>();

nit: full_name could be const

---

systems/analysis/simulator_gflags.cc, line 10 at r2 (raw file):

#include "drake/systems/analysis/simulator_flags.h"

// === Simulator's paramters ===

nit: parameters

[jwnimmer-tri]

Reviewable status: complete! all discussions resolved, LGTM from assignees amcastro-tri,sherm1(platform) (waiting on @amcastro-tri and @sherm1)

---

systems/analysis/simulator_flags.h, line 30 at r2 (raw file):

... consider adding a note on IntegratorBase advising a naming convention that will work here.

Done.

As far what disclaiming which integrators it works for, Doxygen already says "param scheme -- Integration scheme to be used, e.g., "runge_kutta2". See GetIntegrationSchemes() for a the list of valid options.". That's all that matters. This function works fine for, e.g., RadauIntegrator<T, N> which does not follow that pattern, so the disclaimer would be inaccurate.

---

systems/analysis/simulator_flags.h, line 37 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: do you have any particular reason for putting the output parameter (really in/out) first here? If not, better to put it after the input parameters per style guide.

The style guide rule is that input-only params must precede output-only params. The placement of in-out parameters is unspecified.

Here, simulator is like the implicit this in a member method -- the receiver of the call, the object being acted upon -- so it makes sense to me to have it appear first.

[jwnimmer-tri]

The new change in the recent push was making the Radau3/1 aliases public. PTAL.

[sherm1]

That's a good idea.

Reviewed 4 of 4 files at r3.
Reviewable status: complete! all discussions resolved, LGTM from assignees amcastro-tri,sherm1(platform)