refactor AU units #5133

psychocoderHPC · 2024-09-23T11:45:56Z

add conversion au units -> si
add conversion au unis -> pic

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 · 2024-09-23T18:30:51Z

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

massElectron
speedOfLight
simulationTimeStepLength
electronCharge

And for atomicUnits:

atomicUnitEFieldStrength
RydbergEnergy
AtomicUnitTime
(massElectron)

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 · 2024-09-24T06:13:14Z

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 #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.

BrianMarre

Found No bugs, seems to be function wise correct.

include/picongpu/particles/atomicPhysics/initElectrons/Inelastic2BodyCollisionFromCoMoving.hpp

include/picongpu/particles/atomicPhysics/kernel/DecelerateElectrons.kernel

include/picongpu/particles/atomicPhysics/rateCalculation/BoundFreeFieldTransitionRates.hpp

include/picongpu/particles/atomicPhysics/ionizationPotentialDepression/StewartPyattIPD.hpp

share/picongpu/tests/CollisionsBeamRelaxation/include/picongpu/param/particle.param

share/picongpu/tests/CollisionsThermalisation/include/picongpu/param/particle.param

share/picongpu/tests/compileAtomicPhysics/include/picongpu/param/ionizer.param

- add conversion au units -> si - add conversion au unis -> pic ci: picongpu

nvcc is not possible to pull static constexpr member into the device code, therefore acessing a function is required.

- add `automicUnit` - rename `ev` to `eV`

BrianMarre · 2024-09-25T13:08:08Z

...congpu/particles/atomicPhysics/ionizationPotentialDepression/kernel/CalculateIPDInput.kernel

+                    constexpr float_X eV = static_cast<float_X>(sim.si.conv().joule2eV(
+                        picongpu::sim.unit.mass() * pmacc::math::cPow(picongpu::sim.unit.length(), 2u)));


that's a bug, it should read

constexpr float_X eV = static_cast<float_X>(sim.si.conv().joule2eV( picongpu::sim.unit.mass() * pmacc::math::cPow(picongpu::sim.unit.length(), 2u) / pmacc::math::cPow(picongpu::sim.unit.time(), 2u)));

or even shorter

Suggested change

constexpr float_X eV = static_cast<float_X>(sim.si.conv().joule2eV(

picongpu::sim.unit.mass() * pmacc::math::cPow(picongpu::sim.unit.length(), 2u)));

constexpr float_X eV = static_cast<float_X>(picongpu::sim.si.conv().joule2eV(picongpu::sim.unit.energy()));

Should it maybe sim.pic.get_eV(); because I assume you would like to have a picongpu eV?

Yes that is even better!

include/picongpu/particles/atomicPhysics/kernel/DecelerateElectrons.kernel

include/picongpu/particles/atomicPhysics/rateCalculation/BoundBoundTransitionRates.hpp

include/picongpu/particles/atomicPhysics/rateCalculation/CollisionalRate.hpp

BrianMarre · 2024-09-25T14:26:48Z

include/picongpu/unitless/simulation.unitless

+                 * @{
+                 */
+                template<typename T_Type = float_X>
+                constexpr T_Type eV2Joule(T_Type eV = 1.0) const


Suggested change

constexpr T_Type eV2Joule(T_Type eV = 1.0) const

constexpr T_Type eV2picEnergy(T_Type eV = 1.0) const

but be careful when using automated search and replace since eV2Joule also exists in the SI namespace.

also needs to be renamed in the other functions

offline discussed. Other functions are named equally. I am open that someone refactor it in a sperate PR because this is out of the scope of this PR.

BrianMarre · 2024-09-25T14:29:27Z

include/picongpu/plugins/particleCalorimeter/ParticleCalorimeter.x.cpp

+                const float_64 minEnergy_keV = sim.pic.conv().eV2Joule(minEnergy_SI * 1.0e-3);
+                const float_64 maxEnergy_keV = sim.pic.conv().eV2Joule(maxEnergy_SI * 1.0e-3);


bug

Suggested change

const float_64 minEnergy_keV = sim.pic.conv().eV2Joule(minEnergy_SI * 1.0e-3);

const float_64 maxEnergy_keV = sim.pic.conv().eV2Joule(maxEnergy_SI * 1.0e-3);

const float_64 minEnergy_keV = 1.0e-3 * sim.pic.conv().joule2eV(minEnergy_SI);

const float_64 maxEnergy_keV = 1.0e-3 * sim.pic.conv().joule2eV(maxEnergy_SI);

changed it to sim.si.conv()

BrianMarre · 2024-09-25T14:35:58Z

include/picongpu/plugins/particleCalorimeter/ParticleCalorimeter.x.cpp

+            const float_64 minEnergy_SI = this->minEnergy * sim.si.conv().eV2Joule(1.0e3);
+            const float_64 maxEnergy_SI = this->maxEnergy * sim.si.conv().eV2Joule(1.0e3);


bug

Suggested change

const float_64 minEnergy_SI = this->minEnergy * sim.si.conv().eV2Joule(1.0e3);

const float_64 maxEnergy_SI = this->maxEnergy * sim.si.conv().eV2Joule(1.0e3);

const float_64 minEnergy_SI = this->minEnergy * sim.unit.energy();

const float_64 maxEnergy_SI = this->maxEnergy * sim.unit.energy();

it conversion to keV, I will add this to the comment

changed to

/* convert keV to joule */ const float_64 minEnergy_SI = sim.si.conv().eV2Joule(this->minEnergy * 1.0e3); const float_64 maxEnergy_SI = sim.si.conv().eV2Joule(this->maxEnergy * 1.0e3); /* convert into PIConGPU units */ this->minEnergy = minEnergy_SI / sim.unit.energy(); this->maxEnergy = maxEnergy_SI / sim.unit.energy();

BrianMarre

Looks good

BrianMarre

Nope, the FLYonPIC rate calculation unit test fails.

For reference, check by doing the following

pic-create $PIC_EXAMPLES/../tests/compileAtomicPhysics ./testUnitSystem
git clone [email protected]:ComputationalRadiationPhysics/TestUnitSystemAtomicData.git
cd ./testUnitSystem
cp ../TestUnitSystemAtomicData/* ./
pic-build
./bin/picongpu -g 1 1 1 -d 1 1 1 -s 1 --periodic 1 1 1

In ComputationalRadiationPhysics#5131 we changed rydberg energy from eV to joule but missed to apply the conversion in the code.

psychocoderHPC · 2024-09-26T12:29:02Z

fix is applied

The PR ComputationalRadiationPhysics#5133 and ComputationalRadiationPhysics#5142 got merged together with ComputationalRadiationPhysics#4826. Removed files and renamings for the unit systems collide therefore.

psychocoderHPC added component: core in PIConGPU (core application) refactoring code change to improve performance or to unify a concept but does not change public API labels Sep 23, 2024

psychocoderHPC added this to the 0.8.0 / Next stable milestone Sep 23, 2024

psychocoderHPC requested review from PrometheusPi, ikbuibui, steindev and BrianMarre September 23, 2024 11:45

psychocoderHPC force-pushed the topic-refactorAuUNits branch from 6f9b7a8 to d1feb42 Compare September 23, 2024 13:20

psychocoderHPC added the component: user input signals changes in user API such as .param files, .cfg syntax, etc. - changelog! label Sep 23, 2024

psychocoderHPC force-pushed the topic-refactorAuUNits branch from d1feb42 to b52224a Compare September 23, 2024 13:25

psychocoderHPC mentioned this pull request Sep 24, 2024

PIConGPU unit system #5135

Open

BrianMarre approved these changes Sep 24, 2024

View reviewed changes

psychocoderHPC mentioned this pull request Sep 25, 2024

Implementation of Maxwell Juettner distribution #4826

Merged

1 task

psychocoderHPC added 3 commits September 25, 2024 13:19

refactor AU units

17d9f35

- add conversion au units -> si - add conversion au unis -> pic ci: picongpu

change interface conv.* to `conv().*

96f1d9c

nvcc is not possible to pull static constexpr member into the device code, therefore acessing a function is required.

apply review comments

5c99c7c

- add `automicUnit` - rename `ev` to `eV`

psychocoderHPC force-pushed the topic-refactorAuUNits branch from 8637777 to 5c99c7c Compare September 25, 2024 11:19

BrianMarre suggested changes Sep 25, 2024

View reviewed changes

review changes second round

2680519

BrianMarre approved these changes Sep 26, 2024

View reviewed changes

BrianMarre suggested changes Sep 26, 2024

View reviewed changes

fix wrong rydberg energy

9c18c9a

In ComputationalRadiationPhysics#5131 we changed rydberg energy from eV to joule but missed to apply the conversion in the code.

BrianMarre approved these changes Sep 26, 2024

View reviewed changes

BrianMarre mentioned this pull request Sep 26, 2024

add ionization data for argon #5144

Merged

2 tasks

ikbuibui approved these changes Sep 27, 2024

View reviewed changes

ikbuibui merged commit 1e1c7b1 into ComputationalRadiationPhysics:dev Sep 27, 2024
10 checks passed

psychocoderHPC mentioned this pull request Sep 27, 2024

fix MaxwellJuettner functor #5146

Merged

psychocoderHPC deleted the topic-refactorAuUNits branch September 27, 2024 09:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor AU units #5133

refactor AU units #5133

psychocoderHPC commented Sep 23, 2024 •

edited

Loading

BrianMarre commented Sep 23, 2024 •

edited

Loading

psychocoderHPC commented Sep 24, 2024

BrianMarre left a comment

BrianMarre Sep 25, 2024

psychocoderHPC Sep 26, 2024 •

edited

Loading

BrianMarre Sep 26, 2024

BrianMarre Sep 25, 2024

BrianMarre Sep 25, 2024 •

edited

Loading

BrianMarre Sep 25, 2024

psychocoderHPC Sep 26, 2024

BrianMarre Sep 25, 2024

psychocoderHPC Sep 26, 2024

BrianMarre Sep 25, 2024

psychocoderHPC Sep 25, 2024

psychocoderHPC Sep 26, 2024

BrianMarre left a comment

BrianMarre left a comment •

edited

Loading

psychocoderHPC commented Sep 26, 2024

		constexpr float_X eV = static_cast<float_X>(sim.si.conv().joule2eV(
		picongpu::sim.unit.mass() * pmacc::math::cPow(picongpu::sim.unit.length(), 2u)));

	constexpr T_Type eV2Joule(T_Type eV = 1.0) const
	constexpr T_Type eV2picEnergy(T_Type eV = 1.0) const

		const float_64 minEnergy_keV = sim.pic.conv().eV2Joule(minEnergy_SI * 1.0e-3);
		const float_64 maxEnergy_keV = sim.pic.conv().eV2Joule(maxEnergy_SI * 1.0e-3);

		const float_64 minEnergy_SI = this->minEnergy * sim.si.conv().eV2Joule(1.0e3);
		const float_64 maxEnergy_SI = this->maxEnergy * sim.si.conv().eV2Joule(1.0e3);

refactor AU units #5133

refactor AU units #5133

Conversation

psychocoderHPC commented Sep 23, 2024 • edited Loading

Review

BrianMarre commented Sep 23, 2024 • edited Loading

psychocoderHPC commented Sep 24, 2024

BrianMarre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psychocoderHPC Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrianMarre Sep 25, 2024 • edited Loading

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

BrianMarre left a comment

Choose a reason for hiding this comment

BrianMarre left a comment • edited Loading

Choose a reason for hiding this comment

psychocoderHPC commented Sep 26, 2024

psychocoderHPC commented Sep 23, 2024 •

edited

Loading

BrianMarre commented Sep 23, 2024 •

edited

Loading

psychocoderHPC Sep 26, 2024 •

edited

Loading

BrianMarre Sep 25, 2024 •

edited

Loading

BrianMarre left a comment •

edited

Loading