schema: Move rotation and transform out of dev

[jwnimmer-tri]

Important notes for the reviewer: As a "move files out of dev" PR, this only needs platform review, but the reviewer must review all the files as if they were newly written. So the PR is not only a few lines of changes, but rather more like 800 lines.

Towards #13251.

---

This change is 

[EricCousineau-TRI]

First pass done from base..r1. Will come back to it on Monday!

Reviewed 10 of 10 files at r1.
Reviewable status: 11 unresolved discussions, needs platform reviewer assigned, needs at least one assigned reviewer (waiting on @EricCousineau-TRI and @jwnimmer-tri)

---

common/schema/BUILD.bazel, line 16 at r1 (raw file):

drake_cc_package_library(
    name = "schema",
    visibility = ["//visibility:public"],

nit This is redundant w.r.t. above package(default_visibility).

---

common/schema/stochastic.h, line 23 at r1 (raw file):

This page describes how to use classes such as schema::Distribution to denote
stochastic quantities, as bridge between loading a scenario specification and

nit grammar "as a bridge"

---

common/schema/stochastic.h, line 27 at r1 (raw file):

systems::System.

<h1>Stochastic variables</h1>

nit This section is markdown, but these appears to be an unneeded use of HTML.
Use markdown headings?

# Stoachastic variables

---

common/schema/stochastic.h, line 109 at r1 (raw file):

stochastic variables with the same type.

We'll explain uses of schema::DistributionVector and related using the matching

nit grammar "and relates uses using"

---

common/schema/stochastic.h, line 191 at r1 (raw file):

See also

```

nit Trailing space.

---

common/schema/stochastic.h, line 198 at r1 (raw file):

@} */

// N.B. As is standard for classes implementing Serialize(), below we always

nit Per above comment, this seems like it could be an anchored subsection in the above doc, with a cross-reference.

---

common/schema/stochastic.h, line 353 at r1 (raw file):

  virtual ~DistributionVector();

  virtual Eigen::VectorXd Sample(drake::RandomGenerator* generator) const = 0;

nit This repeated contract appears like it could be consolidated across types using a template class, e.g.

template <typename T>
class GenericDistribution {
 public:
  virtual T Sample(drake::RandomGenerator* generator) const = 0;
  ...
}

using Distribution = GenericDistribution<double>;
using DistributionVector = GenericDistribution<Eigen::VectorXd>;

---

common/schema/stochastic.h, line 385 at r1 (raw file):

};

/// A gaussian distribution with vector `mean` and vector or scalar `stddev`.

nit The "vector or scalar stddev" part of this is shown to work through testing (and the implementation).
However, the usage of a 1d vector as a scalar feels "jagged".

Suggestions:

• Remove it; or
• Make the schema accept this strictly as a scalar, not a 1d vector (e.g. Variant<double, VectorX<double>>).

Feel free to dismiss.

---

common/schema/stochastic.h, line 442 at r1 (raw file):

  template <typename Archive>
  void Serialize(Archive*) {
    DRAKE_UNREACHABLE();

nit Since this is a run-time error, is there a way to have this provide more useful debug info? e.g.

template <typename Distribution, int size>
struct InvalidVariantSelection {
  template <typename Archive>
  void Serialize(...) {
    throw std::runtime_error(fmt::format(
      "This schema entry is not intended for scalar-sized distributions ({}). "
      "The defined size is {}.",
      NiceTypeName::Get<Distribution>(), size));
  }
};

---

common/schema/stochastic.h, line 452 at r1 (raw file):

/// Eigen::Dynamic), then this variant also offers the single distributions.
///
/// @tparam Size rows at compile time (max 6) or else Eigen::Dynamic.

nit This should have documentation for the error behavior of using a non-scalar size for scalar-valued distributions. e.g. when does the error occur?

From my current reading, it looks like it occurs if a YAML file requests a scalar distribution like !Gaussian when the schema type is actually something like DistributionVectorVariant<2>.

---

common/schema/dev/rotation.h, line 104 at r1 (raw file):

  }

  // N.B. As is standard for classes implementing Serialize(), we declare all

nit This seems better to live in a separate section, with an explicit cross-reference in documentation.

[jwnimmer-tri]

Oops. There is only one commit in this PR, so you only need to review rbase..rhead. (Some of the files in r1 were no longer in the PR.)

Anyway, maybe it was a good background, and the stochastic code got a bit more tidy for it.

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least one assigned reviewer (waiting on @EricCousineau-TRI)

---

common/schema/BUILD.bazel, line 16 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This is redundant w.r.t. above package(default_visibility).

OK It's required by the macro so that we're explicit. This macro does not inherit the default.

---

common/schema/stochastic.h, line 109 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit grammar "and relates uses using"

Done.

---

common/schema/stochastic.h, line 198 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Per above comment, this seems like it could be an anchored subsection in the above doc, with a cross-reference.

OK I'm going to rewrite all of these (across the tree) in a follow-up PR. The plan is to add a section to name_value.h with guidelines.

---

common/schema/stochastic.h, line 353 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This repeated contract appears like it could be consolidated across types using a template class, e.g.

template <typename T>
class GenericDistribution {
 public:
  virtual T Sample(drake::RandomGenerator* generator) const = 0;
  ...
}

using Distribution = GenericDistribution<double>;
using DistributionVector = GenericDistribution<Eigen::VectorXd>;

OK I don't see how that gains us anything. I don't see that anyone has cause to invoke this base generically. (Dealing with Expression would make the template more complicated anyway.)

---

common/schema/stochastic.h, line 385 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit The "vector or scalar stddev" part of this is shown to work through testing (and the implementation).
However, the usage of a 1d vector as a scalar feels "jagged".

Suggestions:

• Remove it; or
• Make the schema accept this strictly as a scalar, not a 1d vector (e.g. Variant<double, VectorX<double>>).

Feel free to dismiss.

OK Users love this (use it frequently), so it's not going away. It's quite common for a vector to have homogeneous scale, and thus for the stddev to be the same for each element, even with different means (e.g., an RGBA vector).

If I use a variant, we get !VectorX everywhere half the time -- the parser cannot automatically infer vector vs scalar. It's true that the semantics are somewhat non-obvious at a glance, but it's magic we've lived with for two years without any complaints.

I've added more docs here, though, to help readers who look here first.

---

common/schema/stochastic.h, line 442 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Since this is a run-time error, is there a way to have this provide more useful debug info? e.g.

template <typename Distribution, int size>
struct InvalidVariantSelection {
  template <typename Archive>
  void Serialize(...) {
    throw std::runtime_error(fmt::format(
      "This schema entry is not intended for scalar-sized distributions ({}). "
      "The defined size is {}.",
      NiceTypeName::Get<Distribution>(), size));
  }
};

I don't think this code is reachable, at least not with the user being purposefully antagonistic and writing !InvariantVariantSelection in the yaml.

Do you have a hint of how a user (or I) could trigger this in a way that deserves a better error?

---

common/schema/stochastic.h, line 452 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This should have documentation for the error behavior of using a non-scalar size for scalar-valued distributions. e.g. when does the error occur?

From my current reading, it looks like it occurs if a YAML file requests a scalar distribution like !Gaussian when the schema type is actually something like DistributionVectorVariant<2>.

I'm not entirely sure what you're saying.

Would it help if I just listed out two concrete examples? "If Size is 1, here's the list of allowed types. If Size is 3, here's the list of allowed types."

---

common/schema/dev/rotation.h, line 104 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This seems better to live in a separate section, with an explicit cross-reference in documentation.

OK I'm going to rewrite all of these (across the tree) in a follow-up PR. The plan is to add a section to name_value.h with guidelines.

[EricCousineau-TRI]

with a few more remaining comments

Reviewed 10 of 11 files at r2, 2 of 2 files at r3.
Reviewable status: 4 unresolved discussions, needs platform reviewer assigned, needs at least one assigned reviewer (waiting on @jwnimmer-tri)

---

common/schema/stochastic.h, line 109 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Done.

Not done? (I still see "related using")

---

common/schema/stochastic.h, line 353 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK I don't see how that gains us anything. I don't see that anyone has cause to invoke this base generically. (Dealing with Expression would make the template more complicated anyway.)

OK SGTM.

The gain would be keeping the pattern explicit, but yeah, re-templating on Expression would make things painful.

---

common/schema/stochastic.h, line 385 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK Users love this (use it frequently), so it's not going away. It's quite common for a vector to have homogeneous scale, and thus for the stddev to be the same for each element, even with different means (e.g., an RGBA vector).

If I use a variant, we get !VectorX everywhere half the time -- the parser cannot automatically infer vector vs scalar. It's true that the semantics are somewhat non-obvious at a glance, but it's magic we've lived with for two years without any complaints.

I've added more docs here, though, to help readers who look here first.

OK Thanks!

---

common/schema/stochastic.h, line 442 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I don't think this code is reachable, at least not with the user being purposefully antagonistic and writing !InvariantVariantSelection in the yaml.

Do you have a hint of how a user (or I) could trigger this in a way that deserves a better error?

Can you add an explicit negative test for DistributionVectorVariant<2> that shows what the user would encounter in this case?

struct MyThing {
  DistributionVectorVariant<2> value;
};

R"""(
value: !Deterministic { value: 2.0 }
)"""

I assume the error will be something like "!Deterministic is not a supported tag for DistributionVectorVariant<2>"?

---

common/schema/stochastic.h, line 452 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I'm not entirely sure what you're saying.

Would it help if I just listed out two concrete examples? "If Size is 1, here's the list of allowed types. If Size is 3, here's the list of allowed types."

I don't think it needs to be split up, but something that possibly states the obvious:
"Note that non-scalar sizes will not allow users to select the scalar types (Deterministic, Gaussian, Uniform)."

Possibly below the phrase "If the Size parameter [...]" (just to make the negative case more clear...)

---

common/schema/dev/rotation.h, line 104 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK I'm going to rewrite all of these (across the tree) in a follow-up PR. The plan is to add a section to name_value.h with guidelines.

OK Thanks!

---

common/schema/test/transform_test.cc, line 20 at r3 (raw file):

constexpr YamlReadArchive::Options kStrict;

const char* deterministic = R"R(

nit Should use common heredoc, R"""( ... )"""?

(here and elsewhere)

---

common/schema/test/transform_test.cc, line 113 at r3 (raw file):

      drake::math::RollPitchYawd(sampled_transform.rotation())
      .vector() * (180 / M_PI);
  EXPECT_LT(rpy[0], rotation_domain.max[0] - 360);

This seems like a weird place to test wraparound specific to rotation.

Why is this not tested in rotation_test.cc?

---

common/schema/test/transform_test.cc, line 120 at r3 (raw file):

  EXPECT_GT(rpy[2], rotation_domain.min[2]);

  // A second sample will (virtually certainly) differ from the first sample.

BTW "almost certainly" sounds more common and interperetable than "virtually certainly".

Consider simplifying?

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least one assigned reviewer (waiting on @EricCousineau-TRI)

---

common/schema/stochastic.h, line 109 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Not done? (I still see "related using")

Done.

---

common/schema/stochastic.h, line 442 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Can you add an explicit negative test for DistributionVectorVariant<2> that shows what the user would encounter in this case?

struct MyThing {
  DistributionVectorVariant<2> value;
};

R"""(
value: !Deterministic { value: 2.0 }
)"""

I assume the error will be something like "!Deterministic is not a supported tag for DistributionVectorVariant<2>"?

Done.

---

common/schema/stochastic.h, line 452 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I don't think it needs to be split up, but something that possibly states the obvious:
"Note that non-scalar sizes will not allow users to select the scalar types (Deterministic, Gaussian, Uniform)."

Possibly below the phrase "If the Size parameter [...]" (just to make the negative case more clear...)

Done.

---

common/schema/test/transform_test.cc, line 20 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Should use common heredoc, R"""( ... )"""?

(here and elsewhere)

Done (with TODO).

---

common/schema/test/transform_test.cc, line 113 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

This seems like a weird place to test wraparound specific to rotation.

Why is this not tested in rotation_test.cc?

Done (added TODO).

---

common/schema/test/transform_test.cc, line 120 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

BTW "almost certainly" sounds more common and interperetable than "virtually certainly".

Consider simplifying?

Done.

[EricCousineau-TRI]

Reviewed 4 of 4 files at r4.
Reviewable status: needs platform reviewer assigned, needs at least one assigned reviewer

[EricCousineau-TRI]

Oops, looks like Reviewable didn't realized I was assigned:
+@EricCousineau-TRI

Reviewable status: complete! all discussions resolved, LGTM from assignee EricCousineau-TRI(platform)