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

Remove the use of testUtil from within drake/systems #3276

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Aug 27, 2016

This change is Reviewable

@liangfok
Copy link
Contributor

Overall, this PR does a wonderful and significant cleanup.

However, it also switches from measuring execution time in milliseconds to seconds. Has this change impacted measurement precision? It might if the user is saving the measured values in text log files since typically only a limited number of decimal places are saved. Thoughts?

Why not use std::high_resolution_clock()?


Reviewed 20 of 20 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

jwnimmer-tri commented Aug 27, 2016

[This] switches from measuring execution time in milliseconds to seconds.

The units of reporting are changed, but the measurements are still done with sub-second precision. I ran the RBT benchmark before and after this edit, and the results were similar:

Before:

no gradients: 3.336 ms
autodiff fixed max size: 28.663 ms
...

After:

no gradients: 0.00330296 s
autodiff fixed max size: 0.0286834 s
...

Has this change impacted measurement precision? It might if the user is saving the measured values in text log files since typically only a limited number of decimal places are saved. Thoughts?

Given the above sample output, I don't think this is a concern.

Why not use std::high_resolution_clock()?

When I quickly glanced at it, that clock did not appear to be guaranteed to be monotonic.


Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@liangfok
Copy link
Contributor

In the spirit and recognition that we are in a sprint, I bestow upon this PR a :lgtm: under one condition:

  1. Add a CHANGELOG.md entry indicating the API change for measuring method execution times and the fact that the measurement unit was changed from milliseconds to seconds.

Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

Done.


Review status: 20 of 21 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@liangfok
Copy link
Contributor

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 26 [r2] (raw file):

### Changed

 - [#3276][] The measure::execution function has been replaced by MeasureExecutionTime.

BTW: nit: Surround C++ class / method names with back ticks so they appear in a different font, which indicate they are actual software artifacts.

Also add a sentence saying the units changed from milliseconds to seconds.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 26 [r2] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW: nit: Surround C++ class / method names with back ticks so they appear in a different font, which indicate they are actual software artifacts.

Also add a sentence saying the units changed from milliseconds to seconds.

The prior units were undocumented; the new units are documented; the changelog should be as brief as reasonably possible; I don't think the units detail is worth writing about here.

Comments from Reviewable

@liangfok
Copy link
Contributor

Review status: all files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 26 [r2] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The prior units were undocumented; the new units are documented; the changelog should be as brief as reasonably possible; I don't think the units detail is worth writing about here.

OK. SGTM.

Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

Review status: 20 of 21 files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 26 [r2] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

OK. SGTM.

Done (backticks).

Comments from Reviewable

@liangfok
Copy link
Contributor

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@liangfok
Copy link
Contributor

Review status: all files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 26 [r3] (raw file):

### Changed

 - [#3276][] The `measure::execution` function has been replaced by `MeasureExecutionTime`.

BTW: nit: Added () behind method names to indicate that they are method names.


Comments from Reviewable

@liangfok
Copy link
Contributor

liangfok commented Sep 2, 2016

:lgtm:


Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

+@david-german-tri for platform review, please.

@david-german-tri
Copy link
Contributor

:lgtm:


Review status: all files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 39 [r4] (raw file):

 - [#2984][] Renamed and moved `Polynomial.h` and `TrigPoly.h` from `drake/util` to `drake/common` and into the `drakeCommon` library.
 - [#2963][] Rename RigidBody::CollisionElement to RigidBodyCollisionElement.
 - [#3010][] All header file names under `solvers` are now spelled with lower case and underscore names.

why deleted?


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 39 [r4] (raw file):

Previously, david-german-tri (David German) wrote…

why deleted?

It was a duplicate of ~three lines above.

Comments from Reviewable

@jwnimmer-tri jwnimmer-tri merged commit 2470b7f into RobotLocomotion:master Sep 6, 2016
@jwnimmer-tri jwnimmer-tri deleted the systems-nix-testUtil branch September 6, 2016 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants