Implementation of Maxwell Juettner distribution #4826

SergeyKonstantinErmakov · 2024-02-22T18:10:53Z

rebase against dev branch after move PMacc and PIConGPU fully to alpaka #4820 is merged

Velocity distribution for highly relativistic particle temperatures. For this see MaxwellJuettner.hpp and MaxwellJuetner.def.

For verification, a temperature of 10MeV was used.

PrometheusPi · 2024-02-23T10:49:33Z

For some reason, the CI files with some python style issue - code that you did not touch.
I will retrigger the CI.

include/picongpu/particles/manipulators/unary/MaxwellJuettner.hpp

include/picongpu/particles/manipulators/unary/MaxwellJuettner.def

BrianMarre · 2024-02-23T13:41:37Z

@SergeyKonstantinErmakov the CI complains because you are using an old dev as the base for your pull request(PR) and that is what the CI checks.
Please rebase your PR to the current dev.

include/picongpu/particles/manipulators/unary/MaxwellJuettner.hpp

psychocoderHPC · 2024-02-29T08:50:17Z

include/picongpu/particles/manipulators/unary/MaxwellJuettner.hpp

+					u = (-1._X) * theta * math::log(x1 * x2 * x3);
+
+					x4 = uniformNoZeroRng();
+					nu = (-1._X) * theta * log(x1 * x2 * x3 * x4);


bug!!: must be math::log else C function can be used which can have strong side effects

Maybe for performance reasons it is better to write

foo = -1._X * theta; bar = math::log(x1*x2*x3); u = foo * bar; nu = fo * (bar + log(x4);

Thank you very much for the suggestions! Will be implemented.

@psychocoderHPC That still calls log twice, thus no performance gain.

stays the same

PrometheusPi · 2024-02-29T14:13:28Z

@SergeyKonstantinErmakov This pull request failed with a formatting issue, but I do not see in the logs why clang format changed.

PrometheusPi · 2024-02-29T14:19:37Z

For me this error looks like a CI issue and not a common clang format error (which gives the not code style conform code lines). Could you have a look @psychocoderHPC and @chillenzer ?

chillenzer · 2024-02-29T15:07:03Z

Nope, that's a real formatting issue. Following steps will give you a passing CI (see https://picongpu.readthedocs.io/en/latest/dev/docs/COMMIT.html for details):

# install pre-commit if not yet done
cd /path/to/repo
git checkout add_MaxwellJuettner
pre-commit install
pre-commit run --all-files
git add -u
git commit -m "Fix formatting issues"
git push

chillenzer · 2024-02-29T17:44:31Z

PS: As an explanation, clang-format is run within pre-commit since #4817. Which would "report" the necessary changes by editing the files themselves (just like a local run of clang-format would do). In the CI these local changes are wiped out immediately afterwards, so that is not particularly helpful, I agree. pre-commit has better ways to handle this that we currently do not leverage because they are not quite aligned with how we handle CI.

psychocoderHPC · 2024-02-29T19:12:00Z

PS: As an explanation, clang-format is run within pre-commit since #4817. Which would "report" the necessary changes by editing the files themselves (just like a local run of clang-format would do). In the CI these local changes are wiped out immediately afterwards, so that is not particularly helpful, I agree. pre-commit has better ways to handle this that we currently do not leverage because they are not quite aligned with how we handle CI.

@chillenzer can we run git diff in the CI in case a test fails? To show the broken lines?

SergeyKonstantinErmakov · 2024-03-04T12:30:27Z

Thanks to everyone for suggestions and comments! These will be implemented ASAP.

PrometheusPi · 2024-04-09T07:14:50Z

Hi @SergeyKonstantinErmakov, in case you forgot about this pull request: The clang format issue is still open.

PrometheusPi · 2024-05-06T15:38:13Z

Hi @SergeyKonstantinErmakov, what's the status of this pull request?

psychocoderHPC · 2024-05-07T15:34:18Z

@SergeyKonstantinErmakov I removed all files that should not be part of this PR, rebased, formatted the code, and squashed all into a single commit.
I committed all under your credentials.

BrianMarre

in general @SergeyKonstantinErmakov could you rerun the your tests/provide your test scripts to verify the distribution of the current version?

Also the distribution seems to deviate for low energies, do we know why? No Accusation just a a honest question

include/picongpu/particles/manipulators/unary/MaxwellJuettner.def

BrianMarre · 2024-05-08T09:03:43Z

include/picongpu/particles/manipulators/unary/MaxwellJuettner.def

+         * @tparam T_ValueFunctor pmacc::math::operation::*, binary functor type to
+         *                        add a new momentum to an old one
+         */
+        /** @tparam T_ValueFunctor pmacc::math::operation::*, binary functor type to
+         *                        add a new momentum to an old one
+         * @{
+         */


this seems to be doubled up

BrianMarre · 2024-05-08T09:04:42Z

include/picongpu/particles/manipulators/unary/MaxwellJuettner.def

+        /** Version with fixed temperature given via parameter struct
+         *
+         * @tparam T_ParamClass configuration parameter, follows requirements of param::MaxwellJuettnerCfg
+         */
+        /** USE THIS IN THE SAME WAY AS Temperature.hpp JUST REPLACE Temperature with MaxwellJuettner FOR
+         * ALL FUNCTOR AND TEMPLATE NAMES**/


both comment blocks claim to be documentation of something following them, I think you intended them to be one block.

was fixed by @SergeyKonstantinErmakov

BrianMarre · 2024-05-08T09:06:57Z

include/picongpu/particles/manipulators/unary/MaxwellJuettner.def

+        /** Version with fixed temperature given via parameter struct
+         *
+         * @tparam T_ParamClass configuration parameter, follows requirements of param::MaxwellJuettnerCfg
+         */
+        /** USE THIS IN THE SAME WAY AS Temperature.hpp JUST REPLACE Temperature with MaxwellJuettner FOR
+         * ALL FUNCTOR AND TEMPLATE NAMES**/
+        template<typename T_ParamClass, typename T_ValueFunctor>
+        struct MaxwellJuettner;
+
+        /** Version with user-provided temperature functor
+         *
+         * @tparam T_MaxwellJuettnerFunctor temperature functor, follows requirements of
+         * param::MaxwellJuettnerFunctor
+         */
+        template<typename T_Functor, typename T_ValueFunctor>
+        struct FreeMaxwellJuettner;


we should in general think about unifying this param class structure with the existing temperature functor, it does not make that much sense to have tow completely separate temperature param class definitions that are one to one copies.

I agree but let us bring this in and in a separate PR unify it. The user should not see the unification.

include/picongpu/particles/manipulators/unary/MaxwellJuettner.hpp

SergeyKonstantinErmakov · 2024-05-08T11:24:06Z

in general @SergeyKonstantinErmakov could you rerun the your tests/provide your test scripts to verify the distribution of the current version?

Also the distribution seems to deviate for low energies, do we know why? No Accusation just a a honest question

Great Question!
You can find the answer in the paper which I used as a basis for this implementation
https://doi.org/10.1063/1.4919383
under Sobols Method. They explicitly state bad convergence for small energies which essentially comes from one of the approximations for the distribution which they have made (for details see paper), which only agrees for large values. Since Maxwell Juettner is used for high temperatures only, this is not an issue.
I determined that the convergence becomes significantly worse below 10 keV energies. For energies below this level, I suggest using Maxwell Boltzmann.
Maybe this is something which I should document, thanks for pointing that out!

PS: push will follow next thursday :)

BrianMarre · 2024-05-22T15:07:27Z

@SergeyKonstantinErmakov ping!, gentle reminder ;)

PrometheusPi · 2024-06-04T09:50:26Z

@SergeyKonstantinErmakov Could we finish this pull request this week together?

include/picongpu/particles/manipulators/unary/MaxwellJuettner.hpp

steindev · 2024-08-06T12:24:17Z

ping @PrometheusPi

steindev · 2024-09-03T18:51:52Z

ping again @PrometheusPi

psychocoderHPC · 2024-09-19T08:11:28Z

@PrometheusPi If this should be part of the next release we need asap a rebase and update of this PR

steindev · 2024-09-24T09:50:34Z

☝️ @PrometheusPi

PrometheusPi · 2024-09-24T13:46:17Z

@steindev please review then I can squash

PrometheusPi · 2024-09-24T14:45:37Z

@steindev fixed author and squashed - ready for review and merge

PrometheusPi · 2024-09-24T19:16:11Z

@steindev No code changes with the last force push, only linked @SergeyKonstantinErmakov now correctly via his email

psychocoderHPC · 2024-09-25T11:19:25Z

@PrometheusPi you should wait to fix the CI issues until #5133 is merged.
#5133 will have priority because it is more difficulty to rebase it because it is touching the full code base.

…eratures

PrometheusPi · 2024-09-26T10:40:30Z

@steindev The CI discovered a real clang-format issue. This is fixed now. Please review again and merge.

psychocoderHPC · 2024-09-26T12:30:12Z

you can already rebase against #5133 and fix the issues locally

psychocoderHPC · 2024-09-26T12:31:42Z

I chacked the code changes looks like this PR is not affected by #5133

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

PrometheusPi reviewed Feb 23, 2024

View reviewed changes

include/picongpu/particles/manipulators/unary/MaxwellJuettner.hpp Outdated Show resolved Hide resolved

PrometheusPi reviewed Feb 23, 2024

View reviewed changes

include/picongpu/particles/manipulators/unary/MaxwellJuettner.def Outdated Show resolved Hide resolved

steindev requested a review from BrianMarre February 23, 2024 10:52

psychocoderHPC requested changes Feb 29, 2024

View reviewed changes

psychocoderHPC added the component: core in PIConGPU (core application) label Feb 29, 2024

psychocoderHPC added this to the 0.8.0 / Next stable milestone Feb 29, 2024

psychocoderHPC added the changelog PR's marked with this label will be added to the changelog label Feb 29, 2024

SergeyKonstantinErmakov force-pushed the add_MaxwellJuettner branch from 47030d4 to bb4b648 Compare February 29, 2024 12:18

chillenzer mentioned this pull request Mar 1, 2024

Add diffs to CI #4836

Merged

psychocoderHPC force-pushed the add_MaxwellJuettner branch from bb4b648 to 6f3c12b Compare May 7, 2024 15:32

BrianMarre reviewed May 8, 2024

View reviewed changes

PrometheusPi requested changes Jun 10, 2024

View reviewed changes

include/picongpu/particles/manipulators/unary/MaxwellJuettner.hpp Outdated Show resolved Hide resolved

PrometheusPi force-pushed the add_MaxwellJuettner branch 3 times, most recently from 5680c3d to 8897c96 Compare June 24, 2024 07:35

PrometheusPi previously approved these changes Jun 26, 2024

View reviewed changes

steindev reviewed Jun 28, 2024

View reviewed changes

include/picongpu/particles/manipulators/unary/MaxwellJuettner.hpp Show resolved Hide resolved

include/picongpu/particles/manipulators/unary/MaxwellJuettner.hpp Outdated Show resolved Hide resolved

PrometheusPi dismissed stale reviews from psychocoderHPC and themself via 916a8f4 September 24, 2024 12:19

PrometheusPi force-pushed the add_MaxwellJuettner branch from 8897c96 to 916a8f4 Compare September 24, 2024 12:19

PrometheusPi force-pushed the add_MaxwellJuettner branch from 7ffae51 to 13de134 Compare September 24, 2024 14:44

PrometheusPi force-pushed the add_MaxwellJuettner branch 2 times, most recently from 5a07728 to 49a9c7e Compare September 24, 2024 15:45

steindev previously approved these changes Sep 24, 2024

View reviewed changes

PrometheusPi force-pushed the add_MaxwellJuettner branch from 49a9c7e to cb769c6 Compare September 24, 2024 19:15

Maxwell-Juettneri distributions for highly relativistic particle temp…

b7b08d0

…eratures

PrometheusPi dismissed steindev’s stale review via b7b08d0 September 26, 2024 10:39

PrometheusPi force-pushed the add_MaxwellJuettner branch from cb769c6 to b7b08d0 Compare September 26, 2024 10:39

PrometheusPi requested a review from steindev September 26, 2024 11:49

PrometheusPi changed the title ~~Implementation of Maxwell Juettner distribution,~~ Implementation of Maxwell Juettner distribution Sep 26, 2024

steindev approved these changes Sep 27, 2024

View reviewed changes

steindev merged commit 591e4b6 into ComputationalRadiationPhysics:dev Sep 27, 2024
10 checks passed

psychocoderHPC mentioned this pull request Sep 27, 2024

fix MaxwellJuettner functor #5146

Merged

Implementation of Maxwell Juettner distribution #4826

Implementation of Maxwell Juettner distribution #4826

Conversation

SergeyKonstantinErmakov commented Feb 22, 2024 • edited by psychocoderHPC Loading

PrometheusPi commented Feb 23, 2024 • edited Loading

BrianMarre commented Feb 23, 2024

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

PrometheusPi commented Feb 29, 2024

PrometheusPi commented Feb 29, 2024

chillenzer commented Feb 29, 2024

chillenzer commented Feb 29, 2024

psychocoderHPC commented Feb 29, 2024

SergeyKonstantinErmakov commented Mar 4, 2024

PrometheusPi commented Apr 9, 2024

PrometheusPi commented May 6, 2024

psychocoderHPC commented May 7, 2024

BrianMarre 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

SergeyKonstantinErmakov commented May 8, 2024

BrianMarre commented May 22, 2024

PrometheusPi commented Jun 4, 2024

steindev commented Aug 6, 2024

steindev commented Sep 3, 2024

psychocoderHPC commented Sep 19, 2024

steindev commented Sep 24, 2024

PrometheusPi commented Sep 24, 2024

PrometheusPi commented Sep 24, 2024

PrometheusPi commented Sep 24, 2024

psychocoderHPC commented Sep 25, 2024

PrometheusPi commented Sep 26, 2024

psychocoderHPC commented Sep 26, 2024

psychocoderHPC commented Sep 26, 2024

SergeyKonstantinErmakov commented Feb 22, 2024 •

edited by psychocoderHPC

Loading

PrometheusPi commented Feb 23, 2024 •

edited

Loading