ComputationalRadiationPhysics / picongpu

Performance-Portable Particle-in-Cell Simulations for the Exascale Era :sparkles:
https://picongpu.readthedocs.io
Other
705 stars 218 forks source link

refactor AU units #5133

Closed psychocoderHPC closed 1 month ago

psychocoderHPC commented 1 month ago

Please review very carefully!!!!!!

Review

Please check the first commit only. The second commit is changing the access .conv.* to .conv().*. This is required because nvcc is not able to pull the constexpr members correctly into the device code.

BrianMarre commented 1 month ago

Still in the process of the reviewing the PR, but there is one thing I am already really unhappy with. This PR only provides conversion methods for Atomic Units instead of treating AtomicUnits as a separate equally valid third unit system.

This in my eyes the wrong approach and leads to combining orthogonal sets of functions elements into one class and a large number of conversion methods.

The root problem seems to be that the PR tries to press both code internal units and AtomicUnits into one class, although AtomicUnits are logically independent of the code internal units and should therefore be represented as a separate class with it's own conversion functions, instead of mashing them into one general set of conversion functions from SI to whatever.

My suggestion would be to define for internalPicUnits and atomicUnits each a class holding only the 4 base quantities and their SI-values. Namely for internalPicUnits

And for atomicUnits:

and then based on these define for each an daughter class that defines all the derived units in their respective unit systems, e.g. internalPicUnitEField, ... .

In addition it would be nice to define unit prefixes in one central location to provide generic conversion methods to avoid the magic prefix conversion numbers.

psychocoderHPC commented 1 month ago

Still in the process of the reviewing the PR, but there is one thing I am already really unhappy with. This PR only provides conversion methods for Atomic Units instead of treating AtomicUnits as a separate equally valid third unit system.

I agree with you that the current approach is not very nice. It is fully orientated on the current approach. Current normalizations are from a time long before constexpr and other helpful features. This PR is not naming to solve the unit system issue, all the current refactorings have the goal of removing cyclic dependencies or simply removing the *.unitless files.

I opened the issue https://github.com/ComputationalRadiationPhysics/picongpu/issues/5135 for the unit system, but to be fair nobody was interested in this topic until now and without contribution from others I do not see that this will be solved in the next years.

psychocoderHPC commented 1 month ago

fix is applied