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

Basic test for the PileUp #1221

Merged
merged 17 commits into from
Mar 4, 2017
Merged

Basic test for the PileUp #1221

merged 17 commits into from
Mar 4, 2017

Conversation

pleroy
Copy link
Member

@pleroy pleroy commented Mar 3, 2017

No description provided.

@@ -33,7 +33,7 @@ using quantities::Mass;
// A |PileUp| handles a connected component of the graph of |Parts| under
// physical contact. It advances the history and prolongation of its component
// |Parts|, modeling them as a massless body at their centre of mass.
class PileUp final {
class PileUp {
Copy link
Member

Choose a reason for hiding this comment

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

Why not final? In that case, have a virtual destructor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because of TestablePileUp. Done.

// on the origin of |Bubble| given in |Barycentric|, and on the |RigidPileUp|
// degrees of freedom of the parts (as set by |DeformPileUpIfNeeded|).
// Adjusts the degrees of freedom of all parts in this pile up based on the
// position of the pile-up computed by |AdvanceTime| and on the |RigidPileUp|
Copy link
Member

Choose a reason for hiding this comment

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

position? isn't this degrees of freedom?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

0.0 * Metre / Second,
-20.0 / 3.0 * Metre / Second}), 0)));

pile_up.DeformPileUpIfNeeded();
Copy link
Member

Choose a reason for hiding this comment

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

This should not be part of a function that checks invariants.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@eggrobin eggrobin added the LGTM label Mar 4, 2017
@pleroy pleroy merged commit 70d5ec0 into mockingbirdnest:Cardano Mar 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants