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

Rotation cleanup #2382

Merged
merged 10 commits into from
Nov 30, 2018
Merged

Rotation cleanup #2382

merged 10 commits into from
Nov 30, 2018

Conversation

RudolfWeeber
Copy link
Contributor

@RudolfWeeber RudolfWeeber commented Nov 22, 2018

No description provided.

* Remove duplicated frame conversion code
* Make use of Vector3d
@RudolfWeeber
Copy link
Contributor Author

RudolfWeeber commented Nov 23, 2018

Fixes #2383.

The engine tests only check against a stored reference file.
I checked that the engine_lb test passes, if I re-introduce the sign error.
This PR replaced the reference data with data generated with the correct cross product. @hmenke @jdgraaf

@@ -815,7 +816,7 @@ void mpi_send_quat_slave(int pnode, int part) {

/********************* REQ_SET_OMEGA ********/

void mpi_send_omega(int pnode, int part, double omega[3]) {
void mpi_send_omega(int pnode, int part, const double omega[3]) {
Copy link
Member

Choose a reason for hiding this comment

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

const Vector3d& omega

@@ -847,7 +848,7 @@ void mpi_send_omega_slave(int pnode, int part) {

/********************* REQ_SET_TORQUE ********/

void mpi_send_torque(int pnode, int part, double torque[3]) {
void mpi_send_torque(int pnode, int part, const double torque[3]) {
Copy link
Member

Choose a reason for hiding this comment

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

const Vector3d& torque

inline

void
convert_torque_to_body_frame_apply_fix_and_thermostat(Particle &p) {
Copy link
Member

Choose a reason for hiding this comment

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

if there is an “and” in the function name you should think about splitting it

@codecov
Copy link

codecov bot commented Nov 24, 2018

Codecov Report

Merging #2382 into python will increase coverage by <1%.
The diff coverage is 95%.

Impacted file tree graph

@@           Coverage Diff           @@
##           python   #2382    +/-   ##
=======================================
+ Coverage      72%     72%   +<1%     
=======================================
  Files         397     397            
  Lines       18765   18648   -117     
=======================================
- Hits        13571   13487    -84     
+ Misses       5194    5161    -33
Impacted Files Coverage Δ
src/core/communication.hpp 100% <ø> (ø) ⬆️
src/core/rotation.hpp 100% <ø> (ø) ⬆️
src/core/communication.cpp 77% <100%> (-1%) ⬇️
src/core/observables/ParticleBodyVelocities.hpp 100% <100%> (ø) ⬆️
src/core/observables/ParticleAngularVelocities.hpp 100% <100%> (ø) ⬆️
src/core/particle_data.hpp 97% <100%> (ø) ⬆️
src/core/rotate_system.cpp 100% <100%> (ø) ⬆️
src/core/minimize_energy.cpp 100% <100%> (ø) ⬆️
src/core/particle_data.cpp 93% <88%> (ø) ⬆️
src/core/rotation.cpp 98% <93%> (+2%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6db364c...3e97656. Read the comment docs.

} else {
MPI_Send(axis, 3, MPI_DOUBLE, pnode, SOME_TAG, comm_cart);
MPI_Send(axis.data(), 3, MPI_DOUBLE, pnode, SOME_TAG, comm_cart);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use boost::mpi, which will also fix all the builds.

MPI_Recv(axis, 3, MPI_DOUBLE, 0, SOME_TAG, comm_cart, MPI_STATUS_IGNORE);
Vector3d axis;
double angle;
MPI_Recv(axis.data(), 3, MPI_DOUBLE, 0, SOME_TAG, comm_cart,
Copy link
Contributor

Choose a reason for hiding this comment

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

boost::mpi

@@ -95,7 +95,7 @@ bool steepest_descent_step(void) {
}
#ifdef ROTATION
// Rotational increment
double dq[3]; // Vector parallel to torque
Vector3d dq; // Vector parallel to torque
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like can directly initialize dq and remove the loop in line 100.


ONEPART_TRACE(if (p->p.identity == check_id)
fprintf(stderr, "%d: OPT: PPOS p = (%.3f,%.3f,%.3f)\n",
this_node, p->r.p[0], p->r.p[1], p->r.p[2]));
}

inline

Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty line

/** Rotate the particle p around the NORMALIZED axis aSpaceFrame by amount phi
*/
void local_rotate_particle(Particle *p, double *aSpaceFrame, double phi) {
void local_rotate_particle(Particle &p, const Vector3d &a_space_frame,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename a_space_frame to axis.

@RudolfWeeber
Copy link
Contributor Author

Fixes #1318

@RudolfWeeber RudolfWeeber changed the title WIP: Rotation cleanup Rotation cleanup Nov 26, 2018
@fweik fweik dismissed KaiSzuttor’s stale review November 30, 2018 19:22

Points have been addressed.

@fweik fweik merged commit 848a7a8 into espressomd:python Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants