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

Declarative style for particle update comm #2407

Merged
merged 71 commits into from
Feb 4, 2019
Merged

Declarative style for particle update comm #2407

merged 71 commits into from
Feb 4, 2019

Conversation

fweik
Copy link
Contributor

@fweik fweik commented Dec 7, 2018

and reduce duplication.

@fweik fweik requested a review from KaiSzuttor December 7, 2018 17:09
@codecov
Copy link

codecov bot commented Dec 10, 2018

Codecov Report

Merging #2407 into python will decrease coverage by <1%.
The diff coverage is 91%.

Impacted file tree graph

@@           Coverage Diff           @@
##           python   #2407    +/-   ##
=======================================
- Coverage      72%     72%    -1%     
=======================================
  Files         485     484     -1     
  Lines       29926   29509   -417     
=======================================
- Hits        21682   21295   -387     
+ Misses       8244    8214    -30
Impacted Files Coverage Δ
src/core/reaction_ensemble.hpp 68% <ø> (ø) ⬆️
src/core/communication.hpp 100% <ø> (ø) ⬆️
src/core/unit_tests/Vector_test.cpp 100% <100%> (ø) ⬆️
src/core/utils/Span.hpp 100% <100%> (ø) ⬆️
src/core/virtual_sites/VirtualSitesRelative.cpp 82% <100%> (ø) ⬆️
src/core/communication.cpp 66% <100%> (-14%) ⬇️
src/core/reaction_ensemble.cpp 81% <100%> (-1%) ⬇️
src/core/unit_tests/Span_test.cpp 100% <100%> (ø) ⬆️
src/core/utils/Vector.hpp 100% <100%> (ø) ⬆️
src/core/virtual_sites.cpp 83% <100%> (+2%) ⬆️
... and 8 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 4c4b795...ad05e04. Read the comment docs.

@fweik
Copy link
Contributor Author

fweik commented Dec 10, 2018

@KaiSzuttor the AMD issue is unrelated.

@mkuron
Copy link
Member

mkuron commented Dec 10, 2018

AMD fixed in #2412.

@RudolfWeeber
Copy link
Contributor

RudolfWeeber commented Jan 22, 2019

Looks good. A few doc comments would be helpful, though.

diff --git a/src/core/particle_data.cpp b/src/core/particle_data.cpp
+template <typename S, S Particle::*s, typename T, T S::*m>
+struct UpdateParticle {
+  int id;
+  T value;
+
+  void operator()(Particle &p) const { (p.*s).*m = value; }
+
+  template <class Archive> void serialize(Archive &ar, long int) {
+    ar &id;
+    ar &value;
+  }
+};

+
+template <typename T, T ParticleProperties::*m>
+using UpdateProperty = UpdateParticle<ParticleProperties, &Particle::p, T, m>;
+template <typename T, T ParticlePosition ::*m>
+using UpdatePosition = UpdateParticle<ParticlePosition, &Particle::r, T, m>;
+template <typename T, T ParticleMomentum ::*m>
+using UpdateMomentum = UpdateParticle<ParticleMomentum, &Particle::m, T, m>;
+template <typename T, T ParticleForce ::*m>
+using UpdateForce = UpdateParticle<ParticleForce, &Particle::f, T, m>;

Please insert some docs, also with regards to the template params

+template <typename S, S Particle::*s, typename T, T S::*m> struct message_type;

Please also insert some comments here about how the message type distinciotn works.

+

+template <typename S, S Particle::*s, typename T, T S::*m>
+using message_type_t = typename message_type<S, s, T, m>::type;
+
+struct UpdateParticleVisitor : public boost::static_visitor<void> {
+  template <typename Message> void operator()(const Message &msg) const {
+    assert(local_particles[msg.id]);
+    msg(*local_particles[msg.id]);
+  }
+};

Please mention here via what mechanism the actual update gets called since not all readers of this might be familiar with the pattern.

@KaiSzuttor
Copy link
Member

collision detection seems to have issues

@fweik fweik changed the title Declarative style for particle update comm WIP: Declarative style for particle update comm Jan 25, 2019
@fweik
Copy link
Contributor Author

fweik commented Jan 25, 2019

Yeah, I called the master instead of the local interface...

@fweik fweik added the Core label Jan 28, 2019
@fweik fweik changed the title WIP: Declarative style for particle update comm Declarative style for particle update comm Jan 29, 2019
@KaiSzuttor
Copy link
Member

please merge the current python branch

@fweik
Copy link
Contributor Author

fweik commented Jan 29, 2019

@KaiSzuttor

@fweik
Copy link
Contributor Author

fweik commented Feb 4, 2019

@KaiSzuttor I removed the unused return values and cleaned up a bit.

@fweik fweik merged commit 1b0ce9c into espressomd:python Feb 4, 2019
@fweik fweik deleted the particle_update branch February 4, 2019 13:31
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.

4 participants