Pic manager #240

mohsensadr · 2023-11-27T20:29:11Z

Updating the pic-manager in the dev-3.2.0 including:
-making member variables of defined classes private.
-adding get/set functions.
-removing field solver from template parameter of pic-manager.
-revising the MPI calls according to dev-3.2.0.

alpine/FieldContainer.hpp

…nining

mohsensadr · 2023-11-30T09:56:30Z

@srikrrish @matt-frey
Hi, please have a look and let me know if you have any comments/feedback.

srikrrish

If I am not wrong BumponTailInstability still has the old structure. I think it is better to change that as well to be conformant with the other two mini-apps.

…ainers; remove some of this->

mohsensadr · 2023-12-01T12:19:47Z

If I am not wrong BumponTailInstability still has the old structure. I think it is better to change that as well to be conformant with the other two mini-apps.

Added.

alpine/BumponTailInstability.cpp

alpine/BumponTailInstabilityManager.h

srikrrish · 2023-12-07T10:49:19Z

alpine/BumponTailInstabilityManager.h

+constexpr bool EnablePhaseDump = false;
+
+// define functions used in sampling particles
+struct CustomDistributionFunctions {


Why can't this be derived from the Distribution class?

I'm not sure what you mean. The struct "CustomDistributionFunctions" is gonna be used as the template argument to make an instance of Distribution class

using DistR_t = ippl::random::Distribution<double, Dim, 2 * Dim, CustomDistributionFunctions>; ... DistR_t distR(parR);

Ok somehow this is very confusing to me.. In my opinion if a mini-app requires custom distribution it should derive from the base class Distribution and just implement their own PDF, CDF etc. The base class Distribution would then be an abstract class with PDF, CDF etc. as pure virtual functions. This has clear hierarchy in my opinion. Or what problems do you face if you implement in this way?

Initially, we implemented cdf/pdf with function pointers, which wasn't working on GPUs (I was getting illegal memory access, calling host function from a host device function type of error). We had a discussion early on, and the conclusion was that the virtual functions probably won't work on GPUs neither. Since functors work, I ended up with this implementation.

If we want to change the Distribution class again, we should do it in another issue/merge-request I think.

Yeah.. we could try to handle virtual functions on GPUs through https://kokkos.github.io/kokkos-core-wiki/usecases/VirtualFunctions.html , https://github.com/kokkos/kokkos/blob/master/example/virtual_functions/main.cpp and that way we can have the structure as I mentioned. But I agree it is out of the scope of this PR. But something to consider in the future.

matt-frey

I have some requested changes. Also, this PR seems not complete. To me it looks like many basics things have already been merged ... unreviewed.

alpine/BumponTailInstability.cpp

alpine/FieldContainer.hpp

alpine/LandauDampingManager.h

alpine/LoadBalancer.hpp

matt-frey · 2023-12-07T10:52:27Z

src/Manager/FieldSolverBase.h

@@ -62,10 +62,11 @@ using Solver_t = VariantFromConditionalTypes<CGSolver_t<T, Dim>, FFTSolver_t<T,
 namespace ippl {
  template <typename T, unsigned Dim>
  class FieldSolverBase {


I do not yet see the purpose of this abstract base class.

I wanted to remove FieldSolver from the list of template arguments of PicManager. We need a type here to make shared pointer fsolver_m out of it. However, the FieldSolver class is defined in alpine directory. I find two solutions:

We can create a base class for the field solver in src/Manager/. (FieldSolverBase) with very basic member variables. Then, use it to create fsolver_m pointer in pic-manager, and complete/extend it in the FieldSolver class in alpine/ directory by inheritance.

In src/Manager/PicManager.h we can include the field solver class from alpine/FieldSolver.hpp and use the class FieldSolver as a type directly to create fsolver_m pointer.

I implemented the 1st solution, which doesn't require including code from alpine/. directory.
Does it make sense?

Yes, It is a good idea to remove the field solver from the list of template arguments. No, the second solution is definitely not a good idea. I agree with your choice.

From a cursory reading, I also don't understand the point of the FieldSolver(Base) classes. It seems to be that their functionality can be merged into other data types.

It appears that FieldSolverBase is just a wrapper object for a solver. But what semantic meaning does this have? This class is useful if there are many situations where we need another application-generic abstraction layer between the solvers themselves and the application. But the abstraction layer itself doesn't do anything; all the functionality is provided in the ALPINE FieldSolver. As I understand it, conceptually, we have a field solver for the solve step in PIC. But this doesn't require a separate class. The solver is part of the PIC loop and can thus be handled by the PIC manager itself.

The subclass FieldSolver also isn't as generic as the name suggests, or at least its code isn't written this way. The class is clearly written to be a wrapper for an electrostatic solver, given that its fields are referred to as E and rho. The code in this class would be more suited to an ALPINE-level electrostatic PIC manager superclass from which the individual managers are derived. (It's ALPINE-level because the solver initialization uses hard coded settings which might not be adapted to other applications.)

I think these classes are a little redundant, but perhaps introduced because of the complexity of the Solver_t alias. Prior to a clean interface for a shared PIC manager, we need to address #182 and unify the semantic meaning of the template parameters to the solvers. These intermediate solver wrappers provide an interface that would not be needed at all if we could simply have a pointer to Solver be our solver and automatically handle the different solver types.

Once all the solvers have a compatible base type, the PIC manager simply needs a pointer to Solver and all the solver setup becomes generic as well. This eliminates the need for any middle layer handling solver-specific initialization modulo solver settings, e.g. FFT parameters and such.

@matt-frey @srikrrish Do you agree on removing FieldSolver and its base class entirely? Should I implement its variables/methods in the AlpineManager?

srikrrish · 2023-12-14T05:44:26Z

alpine/LandauDampingManager.h

-        Vector_t<double, Dim> hr_m     = hr;
-        Vector_t<double, Dim> origin_m = origin;
-        if ((this->loadbalancethreshold_m != 1.0) && (ippl::Comm->size() > 1)) {
+        Vector_t<double, Dim> kw     = kw_m;


I see in so many places you define new temporary variables. Why are they needed? and why can't you directly use here kw_m? Note these things increase the lines of code (which we want to minimize) and gives an impression of a complex code even though the code itself might be simple only.

Is it because of KOKKOS_LAMBDA vs KOKKOS_CLASS_LAMBDA? In this case we may then want to not specify so many of them in the class and define some of them just locally rather than having it in the class as well as defining a temporary variable.

Yes, I use local variables so that the KOKKOS_LAMBDA doesn't copy all the member variables of the class in the loop. I'll try to reduce local variables as much as possible in the next commit

Yes.. also if that is not possible try reducing the number of variables in the class (if they are not used everywhere) and just have them only defined as local variables

alpine/LandauDampingManager.h

srikrrish

I think we should move UniformPlasmaTest.cpp, ChargedParticles.hpp, LandauDampingMixedExec.cpp and LandauDampingMixedPrecision.cpp to a separate new directory as having them here makes it confusing.

alpine/LandauDampingManager.h

alpine/PenningTrapManager.h

srikrrish · 2023-12-14T06:21:34Z

@Arc676 I requested a review from you as you worked on redesigning the mini-apps interfaces quite extensively. But feel free to skip it if you don't have time which is perfectly fine. Note since some parts of this are already merged if you want to have a full overview unfortunately you have to directly look at the files and also may not be able to directly comment on the lines which you would like to.

Arc676

Some of the code (not necessarily introduced in this PR) is not properly formatted. (I didn't fully read through all the changes in the PR; the changes to the existing managers seem to be interface changes so I just skimmed through those.)

Arc676 · 2023-12-15T02:42:21Z

src/Manager/FieldSolverBase.h

@@ -62,10 +62,11 @@ using Solver_t = VariantFromConditionalTypes<CGSolver_t<T, Dim>, FFTSolver_t<T,
 namespace ippl {
  template <typename T, unsigned Dim>
  class FieldSolverBase {


From a cursory reading, I also don't understand the point of the FieldSolver(Base) classes. It seems to be that their functionality can be merged into other data types.

It appears that FieldSolverBase is just a wrapper object for a solver. But what semantic meaning does this have? This class is useful if there are many situations where we need another application-generic abstraction layer between the solvers themselves and the application. But the abstraction layer itself doesn't do anything; all the functionality is provided in the ALPINE FieldSolver. As I understand it, conceptually, we have a field solver for the solve step in PIC. But this doesn't require a separate class. The solver is part of the PIC loop and can thus be handled by the PIC manager itself.

The subclass FieldSolver also isn't as generic as the name suggests, or at least its code isn't written this way. The class is clearly written to be a wrapper for an electrostatic solver, given that its fields are referred to as E and rho. The code in this class would be more suited to an ALPINE-level electrostatic PIC manager superclass from which the individual managers are derived. (It's ALPINE-level because the solver initialization uses hard coded settings which might not be adapted to other applications.)

I think these classes are a little redundant, but perhaps introduced because of the complexity of the Solver_t alias. Prior to a clean interface for a shared PIC manager, we need to address #182 and unify the semantic meaning of the template parameters to the solvers. These intermediate solver wrappers provide an interface that would not be needed at all if we could simply have a pointer to Solver be our solver and automatically handle the different solver types.

Once all the solvers have a compatible base type, the PIC manager simply needs a pointer to Solver and all the solver setup becomes generic as well. This eliminates the need for any middle layer handling solver-specific initialization modulo solver settings, e.g. FFT parameters and such.

alpine/BumponTailInstabilityManager.h

alpine/LoadBalancer.hpp

alpine/ParticleContainer.hpp

alpine/BumponTailInstabilityManager.h

src/Manager/PicManager.h

mohsensadr · 2023-12-19T09:26:14Z

I think we should move UniformPlasmaTest.cpp, ChargedParticles.hpp, LandauDampingMixedExec.cpp and LandauDampingMixedPrecision.cpp to a separate new directory as having them here makes it confusing.

Done here 1ca0a23
@srikrrish Let me know if you prefer a different name for the directory rather than alpine0

srikrrish · 2023-12-20T12:42:46Z

Regarding the naming for the directory containing the examples without PicManager, at present I also don't have a good solution. But alpine0 sounds confusing. Also this additional directory can still be within the alpine directory itself as they are still alpine related examples. Currently I can only think of naming the directory as ExamplesWithoutPicManager as that is the only common thing among them. May be we just do it like that for now and decide on a better name later.

mohsensadr · 2023-12-20T14:00:39Z

Regarding the naming for the directory containing the examples without PicManager, at present I also don't have a good solution. But alpine0 sounds confusing. Also this additional directory can still be within the alpine directory itself as they are still alpine related examples. Currently I can only think of naming the directory as ExamplesWithoutPicManager as that is the only common thing among them. May be we just do it like that for now and decide on a better name later.

Done here 822aae2 and 8a65884

srikrrish · 2023-12-20T14:30:10Z

@mohsensadr Thanks for incorporating the comments and suggestions. I think some of the (possibly) larger issues in this PR can be addressed separately in other issues in the order of their priority and timing constraints. Hence I am approving it at the moment.

mohsensadr · 2023-12-20T14:37:00Z

@srikrrish No worries and thanks for your patience. I think I can merge only after @matt-frey has also approved it.

Merge branch 'dev-3.2.0' into pic-manager

b823f82

mohsensadr requested review from srikrrish and matt-frey November 27, 2023 20:29

mohsensadr self-assigned this Nov 27, 2023

mohsensadr added the enhancement New feature or request label Nov 27, 2023

aaadelmann approved these changes Nov 28, 2023

View reviewed changes

alpine/FieldContainer.hpp Outdated Show resolved Hide resolved

aaadelmann approved these changes Nov 28, 2023

View reviewed changes

Sadr Mohsen added 3 commits November 29, 2023 12:49

make get/set methods inline

347eeef

make stype_m and stype_m private in FieldSolverBase class; cont. inli…

c5b4ded

…nining

Merge branch 'dev-3.2.0' into pic-manager

f59d716

srikrrish reviewed Nov 30, 2023

View reviewed changes

Sadr Mohsen added 3 commits November 30, 2023 14:23

Remove redundant lines; make P, E, q, public like R; add get for cont…

363090c

…ainers; remove some of this->

added Bump on tail --needs debugging

3c62909

runs on multiple gpus, added dumps

da11d8b

Sadr Mohsen added 3 commits December 1, 2023 15:57

set sd = sigma, minor edits

38bb5eb

init time = 0, dump also in pre-run

aa90206

remove get/set for public P, q, E in particle container

7e84194

mohsensadr requested a review from srikrrish December 1, 2023 16:20

srikrrish requested changes Dec 7, 2023

View reviewed changes

matt-frey requested changes Dec 7, 2023

View reviewed changes

Sadr Mohsen added 3 commits December 7, 2023 15:05

remove PSI's copyright

815d239

add namespace (std) for fabs

9926690

make parR dim ind

ed4edda

mohsensadr requested review from srikrrish and matt-frey December 7, 2023 15:09

add timing

65ba09e

srikrrish requested changes Dec 14, 2023

View reviewed changes

srikrrish self-requested a review December 14, 2023 05:56

srikrrish reviewed Dec 14, 2023

View reviewed changes

alpine/LandauDampingManager.h Outdated Show resolved Hide resolved

alpine/LandauDampingManager.h Outdated Show resolved Hide resolved

alpine/PenningTrapManager.h Outdated Show resolved Hide resolved

srikrrish requested a review from Arc676 December 14, 2023 06:18

Arc676 suggested changes Dec 15, 2023

View reviewed changes

superclass for pic-manager in alpine

b0c0a86

Arc676 reviewed Dec 16, 2023

View reviewed changes

src/Manager/PicManager.h Outdated Show resolved Hide resolved

Sadr Mohsen added 8 commits December 16, 2023 10:01

renaming into alpine manager

39d6f6d

Add def. impl for dump, etc. Add doc

fde22e6

remove inline, fix a bug

dedf58a

Bring mesh, FL in fc; and PL in pc

8fa698a

remove temp var in gather

94ec049

throw error if step method is not set/recognized

d36b610

move dump from PicManager to AlpineManager

af38feb

move prev impl of alpine to alpine0

1ca0a23

mohsensadr requested a review from srikrrish December 19, 2023 09:30

Sadr Mohsen added 2 commits December 20, 2023 14:57

change name/dir to alpine/ExamplesWithoutPicManager for prev. impl.

822aae2

update CMakeLists

8a65884

make grid2par, par2grid, and advance pure virtual methods

000198c

srikrrish approved these changes Dec 20, 2023

View reviewed changes

mohsensadr requested a review from Arc676 December 20, 2023 14:37

srikrrish merged commit 9c43c29 into dev-3.2.0 Dec 20, 2023

srikrrish deleted the pic-manager branch December 20, 2023 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pic manager #240

Pic manager #240

mohsensadr commented Nov 27, 2023

mohsensadr commented Nov 30, 2023

srikrrish left a comment

mohsensadr commented Dec 1, 2023

srikrrish Dec 7, 2023

mohsensadr Dec 7, 2023 •

edited

Loading

srikrrish Dec 7, 2023 •

edited

Loading

mohsensadr Dec 7, 2023

srikrrish Dec 8, 2023

matt-frey left a comment

matt-frey Dec 7, 2023

mohsensadr Dec 7, 2023

matt-frey Dec 14, 2023

Arc676 Dec 15, 2023

mohsensadr Dec 18, 2023

srikrrish Dec 14, 2023

srikrrish Dec 14, 2023

mohsensadr Dec 14, 2023

srikrrish Dec 14, 2023

srikrrish left a comment

srikrrish commented Dec 14, 2023

Arc676 left a comment

Arc676 Dec 15, 2023

mohsensadr commented Dec 19, 2023

srikrrish commented Dec 20, 2023

mohsensadr commented Dec 20, 2023

srikrrish commented Dec 20, 2023

mohsensadr commented Dec 20, 2023

Pic manager #240

Pic manager #240

Conversation

mohsensadr commented Nov 27, 2023

mohsensadr commented Nov 30, 2023

srikrrish left a comment

Choose a reason for hiding this comment

mohsensadr commented Dec 1, 2023

Choose a reason for hiding this comment

mohsensadr Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

srikrrish Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt-frey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikrrish left a comment

Choose a reason for hiding this comment

srikrrish commented Dec 14, 2023

Arc676 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohsensadr commented Dec 19, 2023

srikrrish commented Dec 20, 2023

mohsensadr commented Dec 20, 2023

srikrrish commented Dec 20, 2023

mohsensadr commented Dec 20, 2023

mohsensadr Dec 7, 2023 •

edited

Loading

srikrrish Dec 7, 2023 •

edited

Loading