ippl icon indicating copy to clipboard operation
ippl copied to clipboard

P3M Merge

Open tischwab0911 opened this issue 11 months ago • 2 comments

Pull request to merge the P3M codes into the main repository.

tischwab0911 avatar Feb 11 '25 13:02 tischwab0911

Why did you add the plot's? In general pdfs of results should not be in the repo. Are these plots used in your thesis?

aaadelmann avatar Feb 11 '25 18:02 aaadelmann

  1. Remove src/p3m/ and put P3MParticleContainer together with the high level P3M manager(s) into a alpine/p3m/... folder.

Update after todays IPPL Meeting: Let's not extend "alpine", but rather create a separate directory. Also: I think it would improve clarity if we created a new "examples" directory, where we put all these mini apps in. Maybe a folder structure like the following:

examples/
├─ alpine/
│  ├─ AlpineManager.h
│  ├─ ...
├─ collision/
│  ├─ P3M[...]Manager.hpp
│  ├─ P3MParticleContainer.hpp
│  ├─ datatypes.h
│  ├─ [...].cpp 
│  ├─ ...
├─ cosmology/
│  ├─ ...

Also I just notices you removed P3MParticleContainer.hpp (which is not what I meant above). I was thinking of removing P3M3DManager.h, since it is basically the same as the PicManager.h, but with a fixed particle container type. Since PicManager is templated on the container type, one can easily use it together with P3MParticleContainer.hpp.

@0xBachmann be carefule with the difference between a manager class and a container class ;):

  • Container only saves particle information (e.g. attributes like position, charge, mass) and their layout (e.g. "where on the machine"). On a high level, they should inherit from e.g. ParticleBase.
  • Managers manage the simulation. They hold the containers (particle/field) and all other tools (like the solver instance) and define what happens in the simulation every timestep. On a high level, they should inherit from PicManager for particle simulations.

Therefore, removing the only P3MContainer is not a good idea, it should be high level part of the "P3M mini apps". Removing the code duplication introduced by the P3M3DManager, which does not need to be a high level specialization, however, should improve clarity.

aliemen avatar Mar 25 '25 16:03 aliemen

uups my accept was a mistake, need to revert

aaadelmann avatar Jun 24 '25 15:06 aaadelmann

The P3M code looks good to me, I'll also try to run it in a second.

  • I would add a short description to new classes (just an explanation of what it does and what it is for, similar to what was done here.
  • You added two new functions to ParticleAttrib.h, which I think makes sense. However, since this is very much core-IPPL, you should probably add documentation.

aliemen avatar Jul 07 '25 23:07 aliemen

@matt-frey I requested your review so that you can take a look at the ``ParticleAttrib::internalCopy` function, since you mostly wrote the class, correct?

The function works perfectly fine in my opinion, you don't need to test it. But I think it would make sense to get your opinion on its placement inside the ParticleAttrib class and if you think it belongs there.

aliemen avatar Jul 08 '25 13:07 aliemen

I would also prefer to not have it in the base class, however in ParticleBase all attributes are stored as pointers to ParticleAttribBase and the attribute base class does not provide access to the underlying view (which is not possible as it is dependent on the deriving classes template parameters). Even with forAllAttributes its not possible to reproduce the same behaviour except to serialize, manipulate, deserialize.

0xBachmann avatar Jul 08 '25 14:07 0xBachmann