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.

Sign up for GitHub

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

Jump to bottom

Reduce uses of the local_cells global #2899

Closed
fweik opened this issue Jun 12, 2019 · 10 comments
Closed

Reduce uses of the local_cells global #2899

fweik opened this issue Jun 12, 2019 · 10 comments
Assignees
@fweik
Labels
CodingDay Core

Comments

@fweik
Copy link
Contributor

fweik commented Jun 12, 2019

Many methods directly use the local_cells global, in most of the cases to iterate over the particles.
This exposes implementation details of the cell system to the methods, which isn't actually needed.
For those methods the ParticleRange to be iterated over should be passed as an argument.
@fweik fweik added this to the Espresso 4.1 milestone Jun 12, 2019
@fweik
Copy link
Contributor Author

fweik commented Jun 12, 2019

This can at least be partially automate with tooling, e.g. with clion's extract parameter refactoring. It probably should also be split up, this affects about 150 functions.

@fweik
Copy link
Contributor Author

fweik commented Jun 12, 2019

Example:

int count_particles() {
   int c = 0;
   for(auto const&p : local_cells.particles()) {
      c++;
   }

   return c;
}

should be updated to

int cound_particles(const ParticleRange &particles) {
   int c = 0;
   for(auto const&p : particles) {
      c++;
   }

   return c;
}

@KaiSzuttor KaiSzuttor self-assigned this Jul 16, 2019
@KaiSzuttor KaiSzuttor mentioned this issue Jul 16, 2019
Closed
@KaiSzuttor KaiSzuttor mentioned this issue Jul 25, 2019
Merged
@KaiSzuttor KaiSzuttor removed their assignment Jul 25, 2019
bors bot added a commit that referenced this issue Jul 25, 2019
@bors @christophlohrmann
@jngrad @KaiSzuttor @reinaual @jrfinkbeiner @RudolfWeeber
Merge #2939 #3006 #3008 #3015
4ed35ea 
2939: Documentation of VV, NPT, Thermostats r=RudolfWeeber a=christophlohrmann

Fixes #2680 

Description of changes:
 - Added sphinx documentation for velocity verlet, NPT
- Added/Updated sphinx documentation for Langevin, NPT, LB, DPD thermostat
- Found some papers as reference


PR Checklist
------------
 - [ ] Tests?
   - [ ] Interface
   - [ ] Core 
 - [x] Docs?


3006: Reduce usage of local cells r=fweik a=KaiSzuttor

Fixes part of #2899

Description of changes:
 - replacement for #2991 

3008: Removed from __future__ r=jngrad a=jrfinkbeiner

Fixes #3004

3015: Update checkpointing docs r=jngrad a=RudolfWeeber

Fixes #2808

Co-authored-by: Christoph Lohrmann <clohrmann@icp.uni-stuttgart.de>
Co-authored-by: Jean-Noël Grad <jgrad@icp.uni-stuttgart.de>
Co-authored-by: Kai Szuttor <kai@icp.uni-stuttgart.de>
Co-authored-by: Alexander Reinauer <st144434@stud.uni-stuttgart.de>
Co-authored-by: Jan Finkbeiner <st144298@stud.uni-stuttgart.de>
Co-authored-by: Rudolf Weeber <weeber@icp.uni-stuttgart.de>
bors bot added a commit that referenced this issue Jul 25, 2019
@bors @christophlohrmann
@jngrad @KaiSzuttor @reinaual
Merge #2939 #3006
e98cb15 
2939: Documentation of VV, NPT, Thermostats r=RudolfWeeber a=christophlohrmann

Fixes #2680 

Description of changes:
 - Added sphinx documentation for velocity verlet, NPT
- Added/Updated sphinx documentation for Langevin, NPT, LB, DPD thermostat
- Found some papers as reference


PR Checklist
------------
 - [ ] Tests?
   - [ ] Interface
   - [ ] Core 
 - [x] Docs?


3006: Reduce usage of local cells r=fweik a=KaiSzuttor

Fixes part of #2899

Description of changes:
 - replacement for #2991 

Co-authored-by: Christoph Lohrmann <clohrmann@icp.uni-stuttgart.de>
Co-authored-by: Jean-Noël Grad <jgrad@icp.uni-stuttgart.de>
Co-authored-by: Kai Szuttor <kai@icp.uni-stuttgart.de>
Co-authored-by: Alexander Reinauer <st144434@stud.uni-stuttgart.de>
@RudolfWeeber
Copy link
Contributor

This is merged?

@fweik
Copy link
Contributor Author

fweik commented Aug 6, 2019

This is an issue.

@RudolfWeeber
Copy link
Contributor

RudolfWeeber commented Aug 6, 2019 via email

@fweik
Copy link
Contributor Author

fweik commented Aug 6, 2019

No, of course not?!

@fweik
Copy link
Contributor Author

fweik commented Aug 6, 2019

local_cells.particles() is still used in 157 places.

@fweik
Copy link
Contributor Author

fweik commented Aug 6, 2019

Candidates for refactoring:

pressure.cpp
134:    for (auto &p : local_cells.particles()) {

energy.cpp
106:    for (auto &p : local_cells.particles()) {
112:  auto local_parts = local_cells.particles();

particle_data.cpp
479:  auto end = std::transform(local_cells.particles().begin(),
480:                            local_cells.particles().end(), sendbuf.data(),
518:void build_particle_node() { mpi_who_has(local_cells.particles()); }
1228:  for (auto &p : local_cells.particles()) {
1293:  for (auto &p : local_cells.particles()) {
1320:    for (auto &p : local_cells.particles()) {

cells.cpp
441:  resort_particles |= (std::any_of(local_cells.particles().begin(),
442:                                   local_cells.particles().end(),

EspressoSystemInterface.cpp
36:      copy_part_data_to_gpu(local_cells.particles());

galilei.cpp
59:      local_cells.particles(), std::pair<Utils::Vector3d, double>{},
70:      local_cells.particles(), std::pair<Utils::Vector3d, double>{},
79:  for (auto &p : local_cells.particles()) {

virtual_sites/VirtualSitesInertialessTracers.cpp
37:    IBM_ForcesIntoFluid_GPU(local_cells.particles());
41:  if (std::any_of(local_cells.particles().begin(),
42:                  local_cells.particles().end(),
52:  IBM_UpdateParticlePositions(local_cells.particles());

virtual_sites/VirtualSitesRelative.cpp
34:  for (auto &p : local_cells.particles()) {
132:  for (auto &p : local_cells.particles()) {
161:  for (auto &p : local_cells.particles()) {

virtual_sites/virtual_sites_com.cpp
50:  for (auto &p : local_cells.particles()) {

grid_based_algorithms/lb_particle_coupling.cpp
247:        for (auto &p : local_cells.particles()) {

electrostatics_magnetostatics/p3m-dipolar.cpp
648:  for (auto const &p : local_cells.particles()) {
776:  for (auto &p : local_cells.particles()) {
840:  for (auto &p : local_cells.particles()) {
1161:  auto const n_local_part = local_cells.particles().size();
1170:  for (auto const &p : local_cells.particles()) {
1217:    for (auto &p : local_cells.particles()) {
2123:  for (auto const &p : local_cells.particles()) {

electrostatics_magnetostatics/mmm2d.cpp
595:    for (auto const &p : local_cells.particles()) {
674:    for (auto &p : local_cells.particles()) {
1710:  auto parts = local_cells.particles();

electrostatics_magnetostatics/elc.cpp
168:    for (auto const &part : local_cells.particles()) {
185:    for (auto const &part : local_cells.particles()) {
274:  for (auto &p : local_cells.particles()) {
304:  for (auto const &p : local_cells.particles()) {
338:  for (auto &p : local_cells.particles()) {
365:  for (auto &p : local_cells.particles()) {
452:      for (auto &p : local_cells.particles()) {
472:      for (auto &p : local_cells.particles()) {
526:      for (auto &p : local_cells.particles()) {
539:      for (auto &p : local_cells.particles()) {
562:    for (auto &p : local_cells.particles()) {
595:  for (auto &p : local_cells.particles()) {
701:  for (auto &p : local_cells.particles()) {
791:  for (auto &p : local_cells.particles()) {
824:  for (auto &p : local_cells.particles()) {
877:  for (auto &p : local_cells.particles()) {
992:  for (auto &p : local_cells.particles()) {
1323:  for (auto &p : local_cells.particles()) {
1356:  for (auto &p : local_cells.particles()) {
1390:  for (auto &p : local_cells.particles()) {
1520:  for (auto &p : local_cells.particles()) {
1554:  for (auto &p : local_cells.particles()) {
1590:  for (auto &p : local_cells.particles()) {
1624:  for (auto &p : local_cells.particles()) {

electrostatics_magnetostatics/magnetic_non_p3m_methods.cpp
145:  auto parts = local_cells.particles();
228:  for (auto const &p : local_cells.particles()) {
348:    for (auto &p : local_cells.particles()) {

electrostatics_magnetostatics/p3m.cpp
530:  for (auto &p : local_cells.particles()) {
690:  for (auto &p : local_cells.particles()) {
926:  for (auto const &p : local_cells.particles()) {
943:    for (auto &p : local_cells.particles()) {
1991:  for (auto const &p : local_cells.particles()) {

electrostatics_magnetostatics/icc.cpp
204:  for (auto &p : local_cells.particles()) {

electrostatics_magnetostatics/mdlc_correction.cpp
56:      local_cells.particles().begin(), local_cells.particles().end(), 0.0,
83:  for (auto const &p : local_cells.particles()) {
145:        for (auto const &p : local_cells.particles()) {
185:        for (auto &p : local_cells.particles()) {
237:  for (auto const &p : local_cells.particles()) {
279:  n_local_particles = local_cells.particles().size();
305:        for (auto const &p : local_cells.particles()) {
353:  n_local_particles = local_cells.particles().size();
383:  for (auto &p : local_cells.particles()) {

electrostatics_magnetostatics/scafacos.cpp
88:  for (auto const &p : local_cells.particles()) {
115:  for (auto &p : local_cells.particles()) {

io/writer/h5md_core.cpp
444:    for (auto &current_particle : local_cells.particles()) {

@reinaual reinaual mentioned this issue Aug 8, 2019
Merged
@KaiSzuttor KaiSzuttor removed this from the Espresso 4.1 milestone Aug 9, 2019
bors bot added a commit that referenced this issue Aug 9, 2019
@bors @reinaual @jngrad
Merge #3065
74b63f5 
3065: Reduce usage of local_cells r=jngrad a=reinaual

Fixes part of #2899


Co-authored-by: Alexander Reinauer <st144434@stud.uni-stuttgart.de>
Co-authored-by: Jean-Noël Grad <jgrad@icp.uni-stuttgart.de>
@jngrad
Copy link
Member

jngrad commented Aug 9, 2019

thanks to @reinaual there are only 29 occurrences left:

src/core/EspressoSystemInterface.cpp
36:      copy_part_data_to_gpu(local_cells.particles());

src/core/galilei.cpp
59:      local_cells.particles(), std::pair<Utils::Vector3d, double>{},
70:      local_cells.particles(), std::pair<Utils::Vector3d, double>{},
79:  for (auto &p : local_cells.particles()) {

src/core/energy.cpp
100:  calc_long_range_energies(local_cells.particles());
102:  auto local_parts = local_cells.particles();

src/core/cells.cpp
420:  on_resort_particles(local_cells.particles());
451:  resort_particles |= (std::any_of(local_cells.particles().begin(),
452:                                   local_cells.particles().end(),

src/core/pressure.cpp
135:  calc_long_range_virials(local_cells.particles());

src/core/particle_data.cpp
479:  auto end = std::transform(local_cells.particles().begin(),
480:                            local_cells.particles().end(), sendbuf.data(),
518:void build_particle_node() { mpi_who_has(local_cells.particles()); }
1228:  for (auto &p : local_cells.particles()) {
1293:  for (auto &p : local_cells.particles()) {
1320:    for (auto &p : local_cells.particles()) {

src/core/virtual_sites/VirtualSitesInertialessTracers.cpp
37:    IBM_ForcesIntoFluid_GPU(local_cells.particles());
41:  if (std::any_of(local_cells.particles().begin(),
42:                  local_cells.particles().end(),
52:  IBM_UpdateParticlePositions(local_cells.particles());

src/core/virtual_sites/VirtualSitesRelative.cpp
34:  for (auto &p : local_cells.particles()) {
132:  for (auto &p : local_cells.particles()) {
161:  for (auto &p : local_cells.particles()) {

src/core/electrostatics_magnetostatics/scafacos.cpp
88:  for (auto const &p : local_cells.particles()) {
115:  for (auto &p : local_cells.particles()) {

src/core/electrostatics_magnetostatics/mdlc_correction.cpp
55:  auto local_particles = local_cells.particles();

src/core/electrostatics_magnetostatics/p3m-dipolar.cpp
2129:  for (auto const &p : local_cells.particles()) {

src/core/electrostatics_magnetostatics/p3m.cpp
1996:  for (auto const &p : local_cells.particles()) {

src/script_interface/h5md/h5md.cpp
37:    m_h5md->Write(m_h5md->what(), partCfg(), local_cells.particles());

@jngrad jngrad mentioned this issue Feb 6, 2020
Closed
@fweik fweik self-assigned this Feb 11, 2020
@fweik
Copy link
Contributor Author

fweik commented Feb 11, 2020 •
edited
Loading

The following places can be kept until we have decided how to pass global state to
callbacks:

  • src/core/galilei.cpp
  • src/core/particle_data.cpp
  • src/core/electrostatics_magnetostatics/mdlc_correction.cpp
  • src/script_interface/h5md/h5md.cpp
  • src/core/electrostatics_magnetostatics/p3m.cpp
  • src/core/electrostatics_magnetostatics/p3m-dipolar.cpp

The following places do actually depend on the cell system (not only on the particles):

  • src/core/energy.cpp
  • src/core/pressure.cpp

The following usage can be fixed by an interface change:

  • src/core/virtual_sites/VirtualSitesRelative.cpp
  • src/core/EspressoSystemInterface.cpp

The following sites are flagged for removal and can be ignored:

  • src/core/virtual_sites/VirtualSitesInertialessTracers.cpp

@jngrad jngrad mentioned this issue Feb 19, 2020
Open
@fweik fweik mentioned this issue Apr 2, 2020
Merged
kodiakhq bot added a commit that referenced this issue Apr 3, 2020
@kodiakhq
Local particles (#3625)
cc2d279 
The goal here is to make it transparent which call sites actually need access to
the cells, and which just need to iterate the particles. There are a quite a lot of
changes her, but most of them are just textual replacements, so should be fast
to check. The final goal here is to be able to hide the cells from client code of
the particle storage. From my side #2899 can be closed with this.

Description of changes:
- Add functions {local_particles, ghost_particles} to CellStructure to directly get the local/ghost particle ranges
- Replaced all occurrences of particles access thru the {local, ghost} cell ranges by
  direct calls to the aforementioned functions
- Replaced CellPList by the now functionally equivalent Utils::Span
@fweik fweik closed this as completed Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fweik fweik

Labels
CodingDay Core
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@RudolfWeeber @fweik @KaiSzuttor @jngrad