schema: Add stochastic types #13295
schema: Add stochastic types #13295
Conversation
|
+@ggould-tri for feature review of the move, please. Note that reviewable has r1 for the straight copy and r2+ for the changes. |
c193e71 to
8f13490
Compare
There was a problem hiding this comment.
Reviewed 2 of 4 files at r2, 2 of 2 files at r3.
Reviewable status: 1 unresolved discussion, labeled "do not merge" (waiting on @jwnimmer-tri)
a discussion (no related file):
Not sure if this PR is the place to address this or if this comment should be saved for the move out of dev, or handled alongside the port of Transform; please dismiss this if you think it's premature for this commit.
The documentation on DistributionVector / DistributionVectorVariant isn't really adequate to Drake standards. It's misleadingly minimal and doesn't capture the subtlety of the semantics, in particular with respect to the homogeneity of the vector -- it is really a distribution over vectors more than a vector of distributions, since you can't have a DistributionVector that is [Gaussian, Gaussian, Uniform] or similarly non-homogeneous. This will become acute as people try to create stochastic free body states where the translational and rotational coordinates require different approaches: The type names will become the classic "maze of twisty little passages, all alike" in the style of BasicVector/VectorBase unless there is a worked example in hand for them to copy from that will avoid ever touching those pitfalls.
(Of course the solution to that depends on the plans for bringing Transform over: If you plan to bring Transform over then its documentation will become the most acute place where this information is missing)
I think this class really needs a more doxygen-oriented explanation that includes examples. But possibly not in this PR -- a TODO would be reasonable perhaps?
Co-authored-by: Grant Gould <ggould@tri.global> Co-authored-by: Sam Creasey <sam.creasey@tri.global>
Adjust constructors for clang++ compability.
8f13490 to
e39c832
Compare
There was a problem hiding this comment.
Reviewable status: labeled "do not merge" (waiting on @ggould-tri)
a discussion (no related file):
Previously, ggould-tri wrote…
Not sure if this PR is the place to address this or if this comment should be saved for the move out of dev, or handled alongside the port of Transform; please dismiss this if you think it's premature for this commit.
The documentation on DistributionVector / DistributionVectorVariant isn't really adequate to Drake standards. It's misleadingly minimal and doesn't capture the subtlety of the semantics, in particular with respect to the homogeneity of the vector -- it is really a distribution over vectors more than a vector of distributions, since you can't have a DistributionVector that is [Gaussian, Gaussian, Uniform] or similarly non-homogeneous. This will become acute as people try to create stochastic free body states where the translational and rotational coordinates require different approaches: The type names will become the classic "maze of twisty little passages, all alike" in the style of BasicVector/VectorBase unless there is a worked example in hand for them to copy from that will avoid ever touching those pitfalls.
(Of course the solution to that depends on the plans for bringing Transform over: If you plan to bring Transform over then its documentation will become the most acute place where this information is missing)
I think this class really needs a more doxygen-oriented explanation that includes examples. But possibly not in this PR -- a TODO would be reasonable perhaps?
Done (via TODO).
We have separate xyx and rpy already in transform, so I'm not sure that I understand the full worry yet, but I agree we can add more docs.
Towards #13251.
This change is