Refactor ionization: remove from Particles class member functions

[n01r]

Formerly ionization was started from a Particles class member function void ionize(). This seems unnatural, though, since ionization should start from an ionization model and be performed on the various particle species.

It was thus refactored and now allows for a more variable ionization routine. Different models can call kernels with or without loading random generators, for example.

[PrometheusPi]

please update the license years 😄

[n01r]

done ^^;

[n01r]

@ax3l @psychocoderHPC Would you kindly (quote ... unquote) have a look? 😄

[n01r]

What if my ADK implementation would need the kernel call to look like this:

/* random number generator for Monte Carlo */
typedef particles::ionization::RandomNrForMonteCarlo<T_SrcSpecies> T_RandomGen;

T_RandomGen RandomGen(currentStep);

/* kernel call : instead of name<<<blocks, threads>>> (args, ...) 
 * "blocks" will be calculated from "this->cellDescription" and "CORE + BORDER" 
 * "threads" is calculated from the previously defined vector "block" */
__picKernelArea( particles::ionization::kernelIonizeParticles<BlockArea>, *cellDescription, CORE + BORDER )
                    (block)
                    ( src.getDeviceParticlesBox( ),
                      electrons->getDeviceParticlesBox( ),
                      fieldE->getDeviceDataBox( ),
                      fieldB->getDeviceDataBox( ),
                      FrameIonizer( ),
                      RandomGen
                      );

?

Do you think I should restructure the implementation more or is there an elegant solution to handle that the kernel sometimes gets a random generator as an argument and sometimes not?

[psychocoderHPC]

Yes please restructure, the kernel kernelIonizeParticles should nothing know about RandomGen.

[psychocoderHPC]

This method is equal for all Ionizer functors (methods). Please move it out of BSI, it is the same for ADK.

[psychocoderHPC]

Please not mix host functors and device functor. Every functor had only one task.

Currently BSI had the expression how the functor on device is started (kernel call) and the device implementation.

[n01r]

But since the kernel cannot know about a random generator, the implementation of FrameIonizer should. That again means, though, that the implementation is different for BSI and ADK.

[psychocoderHPC]

I think both BSI and ADK kernel call are the same. You can also skip the fields in the kernel call, this is a dependency of the solver. Not every solver needs B and E fields.

The pure kernel call can look like this.

__picKernelArea( particles::ionization::kernelIonizeParticles<BlockArea>, *cellDescription, CORE + BORDER )
                   (block)
                   ( src.getDeviceParticlesBox( ),
                     electrons->getDeviceParticlesBox( ),
                     AnyIonizer( )
                     );

You can find an example here The functors itself can also have a RNG or field inside but the pure kernel nothing knows about this.
Functor with RNG
Functor with field inside

[n01r]

@psychocoderHPC, @ax3l
Took me a while but finally I pulled it together again.
Thx a bunch to @Flamefire

[psychocoderHPC]

As suggestion: add currentStep to the constructor. It can be useful to initialize a random number generator

[n01r]

true, already implemented in a different branch, so it should be added here, too

[psychocoderHPC]

should

{
}

[PrometheusPi]

@n01r Could you mention that this solves your chicken-and-egg problem here?

[PrometheusPi]

👍 done

[PrometheusPi]

@n01r great job 👍

If you fix the above issues, I am fine with this pull request.
@ax3l wanted to have a look at it till Friday - so we have to wait a bit till we can merge.

[PrometheusPi]

@n01r Thank you for the small fixes.

[n01r]

I ran a small test setup of two simulations with the same paramSets.
They took the same amount of time and had the same number of particles per GPU (roughly 18 million). The picture above shows the difference generated from the png-output of the simulations at step 960/1000. Only in three spots there seems to be a visible difference in electron density of about 20%.

[ax3l]

looks quite good, well done.

@n01r for (future) comparisons, just dump a h5 file and access the fields directly via h5py. the png preview is not a reliable source.

[ax3l]

just use the doxygen syntax we usually use:

/** Headline
 *
 * longer description
 * 
 * \tparam T_SpeciesName [... description ...]
 */

Writing the name of the class as the headline is not ideal, it does not add documentation value. or did doxygen noch realize this lines are for the following class?

in that case it's

/** \struct ClassName
 *
 * \brief longer description
 * 
 * \tparam T_SpeciesName [... description ...]
 */

[n01r]

Yeah it does realize that the documentation is for the following class
Actually it is:

/** \struct ClassName
 *
 * \brief shorter description (one-liner maybe)   
 *   
 * longer description ... blabla
 *  
 * \tparam T_SpeciesName [... description ...]
 */

I just skipped the longer one

[ax3l]

yes, you only need those if not put directly over the classes (which would be messy).

the only thing one needs of those is actually \file for a general file description.

[n01r]

so we should maybe add file descriptions in a separate pull request?

[ax3l]

ok I have some small annotations, besides these it looks great to me!

[ax3l]

@PrometheusPi If you are done with your part of the review I can take it over and overview the final changes.

[n01r]

@ax3l
I pushed the changes

[ax3l]

great, very well done! ✨

[bussmann]

Nice!

[psychocoderHPC]

This typedef is not following our naming conventions and are unused.

Please remove both lines