Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support default scalars for TrajectorySource #19967

Merged

Conversation

RussTedrake
Copy link
Contributor

@RussTedrake RussTedrake commented Aug 9, 2023

+@ggould-tri for feature review, please.

N.B. -- My first draft used if_then_else in DoCalcVectorOutput. I also considered making the constructor take optional<bool> zero_derivatives_beyond_limits, and which could default to false for Expression. But I currently think this simpler approach is best.


This change is Reviewable

@RussTedrake RussTedrake added the release notes: feature This pull request contains a new feature label Aug 9, 2023
@RussTedrake RussTedrake force-pushed the trajectory_source_scalars branch from e7e35c3 to 5376ad5 Compare August 9, 2023 01:45
Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

I tried converting the test to use `TYPED_TEST` to prove to myself that the whole test suite works over scalars; I wasn't really able to make any progress with that. If you would like to take a run at that, it might give a superior test suite; I can send you my diff. But this PR seems adequate as-is -- there's not enough interface space for it to be wrong.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: needs at least two assigned reviewers

@jwnimmer-tri jwnimmer-tri self-assigned this Aug 9, 2023
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: 3 unresolved discussions

a discussion (no related file):

N.B. -- My first draft used if_then_else in DoCalcVectorOutput.

Could you write down your thoughts on why this approach was no good?

In case the time in the context was a constant, the if_then_else would evaporate, leaving just the symbolic trajectory. In case time was a variable, we would be

To aid in discussion, I'll number the options:

(1) Use if_then_else in DoCalcVectorOutput.

(2) zero_derivatives_beyond_limits defaults to false for T=Expression, but remains true for other scalar types. (We would do this with constructor overloading, not optional<bool>.)

There are also more options:

(3) Have the constructor throw in when we have all of T=Expression and output_derivative_order > 0 and zero_derivatives_beyond_limits=True. In other words, promote the console warning to an error.

(4) Disable the zero_derivatives_beyond_limits constructor argument when T=Expression, documenting that it defaults to false. In other words, promote the console warning to a compilation error.



systems/primitives/trajectory_source.h line 29 at r1 (raw file):

/// @endsystem
///
/// @tparam_default_scalar

nit It's probably worth somewhere in the class overview documentation explaining that even though this system supports all scalar types, it does not support scalar conversion. (This is atypical.)


systems/primitives/trajectory_source.cc line 58 at r1 (raw file):

    // zero_derivatives_beyond_limits is true by default, but presumably most
    // users will not want the clamped derivatives for symbolic types.
    log()->warn(

nit Logging in a Calc function risks having the warning spam happen at the tick rate of of a simulation, or perhaps more likely at the NLP solver evaluation step rate.

I think we need to use warn-once here instead:
https://drake.mit.edu/doxygen_cxx/structdrake_1_1logging_1_1_warn.html

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 unresolved discussions

a discussion (no related file):

In case time was a variable, we would be ...

Meant to say: in case time was a variable (and zero_derivatives_beyond_limits was true and there were derivatives requested), the output would match up between the scalar types. This will matter if we ever decide to add scalar conversion.


Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 unresolved discussions

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

In case time was a variable, we would be ...

Meant to say: in case time was a variable (and zero_derivatives_beyond_limits was true and there were derivatives requested), the output would match up between the scalar types. This will matter if we ever decide to add scalar conversion.

I think my TLDR here is "a console warning is not acceptable". In the case in question (T=Expression, order > 0, zero_beyond=true), we either need to fail-fast at compile-time or run-time, or else we need to obey the flag. Ignoring the flag doesn't sit well with me.



systems/primitives/trajectory_source.h line 29 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit It's probably worth somewhere in the class overview documentation explaining that even though this system supports all scalar types, it does not support scalar conversion. (This is atypical.)

(If you think it would help, I could follow up with another PR to enable scalar conversion.)


systems/primitives/trajectory_source.cc line 58 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Logging in a Calc function risks having the warning spam happen at the tick rate of of a simulation, or perhaps more likely at the NLP solver evaluation step rate.

I think we need to use warn-once here instead:
https://drake.mit.edu/doxygen_cxx/structdrake_1_1logging_1_1_warn.html

(Given the other discussion threads, this will probably become moot.)

Copy link
Contributor Author

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 unresolved discussions

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think my TLDR here is "a console warning is not acceptable". In the case in question (T=Expression, order > 0, zero_beyond=true), we either need to fail-fast at compile-time or run-time, or else we need to obey the flag. Ignoring the flag doesn't sit well with me.

My main concern was that the most common route to obtaining the TrajectorySource<Expression> is via TrajectorySource<double>::ToExpression(). In this case, the defaults will have zero_derivatives_beyond_limits = true, and in all of your proposals, the resulting Expression will not be what the user wants/expects.

But I read the code and see now that, as you say, if_then_else should return only the simpler expression in the case where time is a double. That's pretty good! If it's only when people set time=Expression with variables that they get something more complicated, then I'm happier with that being the "default" behavior in this code path.



systems/primitives/trajectory_source.h line 29 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

(If you think it would help, I could follow up with another PR to enable scalar conversion.)

I see in https://drakedevelopers.slack.com/archives/C2CHRT98E/p1692059337242299?thread_ts=1692048964.973159&cid=C2CHRT98E that your idea is to scalar convert the system, but leave the underlying Trajectory as double (until updated). That seems very reasonable. I was thinking I also needed to scalar convert the trajectory in the same step... but I agree that the two can potentially be separated.

@RussTedrake RussTedrake force-pushed the trajectory_source_scalars branch from 5376ad5 to 5985465 Compare August 15, 2023 12:14
Copy link
Contributor Author

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion

a discussion (no related file):

Previously, RussTedrake (Russ Tedrake) wrote…

My main concern was that the most common route to obtaining the TrajectorySource<Expression> is via TrajectorySource<double>::ToExpression(). In this case, the defaults will have zero_derivatives_beyond_limits = true, and in all of your proposals, the resulting Expression will not be what the user wants/expects.

But I read the code and see now that, as you say, if_then_else should return only the simpler expression in the case where time is a double. That's pretty good! If it's only when people set time=Expression with variables that they get something more complicated, then I'm happier with that being the "default" behavior in this code path.

PTAL.



systems/primitives/trajectory_source.h line 29 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

I see in https://drakedevelopers.slack.com/archives/C2CHRT98E/p1692059337242299?thread_ts=1692048964.973159&cid=C2CHRT98E that your idea is to scalar convert the system, but leave the underlying Trajectory as double (until updated). That seems very reasonable. I was thinking I also needed to scalar convert the trajectory in the same step... but I agree that the two can potentially be separated.

Done. PTAL.


systems/primitives/trajectory_source.cc line 58 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

(Given the other discussion threads, this will probably become moot.)

Done n/a.

@RussTedrake RussTedrake force-pushed the trajectory_source_scalars branch 2 times, most recently from 86387ba to ecc99b7 Compare August 15, 2023 13:23
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 4 files at r2, 1 of 2 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions

a discussion (no related file):
BTW See RussTedrake#53:

  • Clarify in the docs (and conversion traits) that conversion is from-double only.
  • Explicitly write down the representation invariants.
  • Reduce duplication in output calc.
  • Enable cloning (when T=double).
  • Use THROW (not DEMAND) for user mistakes.
  • Clarify error messages.


systems/primitives/trajectory_source.h line 45 at r3 (raw file):

  /// @param zero_derivatives_beyond_limits All derivatives will be zero before
  /// the start time or after the end time of @p trajectory. However, this
  /// clamping is ignored for T=Expression.

nit Vestigial docs

Suggestion:

  /// the start time or after the end time of @p trajectory.

@RussTedrake RussTedrake force-pushed the trajectory_source_scalars branch from de02336 to 85f9aa3 Compare August 16, 2023 09:10
Copy link
Contributor Author

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW See RussTedrake#53:

  • Clarify in the docs (and conversion traits) that conversion is from-double only.
  • Explicitly write down the representation invariants.
  • Reduce duplication in output calc.
  • Enable cloning (when T=double).
  • Use THROW (not DEMAND) for user mistakes.
  • Clarify error messages.

Grabbed it. Thanks.



systems/primitives/trajectory_source.h line 45 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Vestigial docs

Done.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion

a discussion (no related file):
@ggould-tri please refresh your feature review and re-LGTM, when possible.


Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: 2 unresolved discussions

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

@ggould-tri please refresh your feature review and re-LGTM, when possible.

Tickybox. ☑️
:lgtm:



systems/primitives/trajectory_source.h line 33 at r4 (raw file):

/// the stored Trajectory is not automatically scalar converted. You must call
/// UpdateTrajectory() with an updated Trajectory<T> in order to fully-enable
/// scalar-type support on the trajectory parameters/values.

typo: no hyphen after 'ly'

Also "scalar type" is not hyphenated elsewhere in this file but the codebase on the whole is inconsistent on this point so no defect.

Suggestion:

/// UpdateTrajectory() with an updated Trajectory<T> in order to fully enable
/// scalar-type support on the trajectory parameters/values.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)

@jwnimmer-tri jwnimmer-tri merged commit 6c1e6a7 into RobotLocomotion:master Aug 16, 2023
@jwnimmer-tri jwnimmer-tri added the status: squashing now https://drake.mit.edu/reviewable.html#curated-commits label Aug 16, 2023
@RussTedrake RussTedrake deleted the trajectory_source_scalars branch August 17, 2023 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: feature This pull request contains a new feature status: squashing now https://drake.mit.edu/reviewable.html#curated-commits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants