Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add laser parameter output for --dump-metadata #4938

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

max-lehman14
Copy link
Contributor

@max-lehman14 max-lehman14 commented Jun 12, 2024

This pull request adds laser parameter output in the BaseParam.def and GaussianPulse.def files, based on the new parameter output feature from @chillenzer .

In the picongpu-metadata.json file the standard laser parameters from incidentField.param are displayed with units, for example:

"W0": {
    "unit": "m",
    "value": 4.246609082647506e-06
}

or

"direction": {
    "unit": "none",
    "value": [
      0.0,
      1.0,
      0.0
    ]
},

The next things on my list are:

  • adding README [EDIT/REMOVED by @PrometheusPi]

  • invest deeper in the problem, that Laguerremodes and -phases in the incidentField.param file are not correctly displayed if there are several Laguerremodes (seems like its falling back to the default 0th Laguerre mode for a standard Gaussian)

@chillenzer
Copy link
Contributor

chillenzer commented Jun 12, 2024

Without having looked at the content I can already see that this file didn't change, so almost certainly you didn't run the tests (as the failing CI is also telling you). Please update said file and check that the tests and examples all compile and pass.

@PrometheusPi PrometheusPi added refactoring code change to improve performance or to unify a concept but does not change public API labels Jun 26, 2024
@PrometheusPi PrometheusPi added this to the 0.8.0 / Next stable milestone Jun 26, 2024
@chillenzer
Copy link
Contributor

@PrometheusPi @max-lehman14 What's the status here?

@PrometheusPi
Copy link
Member

@chillenzer I will take over this PR

@PrometheusPi
Copy link
Member

This still does not output anything else than the default parameters. And there should be a bug in the Gaussian code that is actually never causing any compile time issues.

@PrometheusPi
Copy link
Member

@steindev @chillenzer @psychocoderHPC ready for review
I will not add a README since I do not see the benefit of that. If you see any, please let m eknow.

@PrometheusPi PrometheusPi marked this pull request as ready for review September 26, 2024 11:26
Copy link
Member

@steindev steindev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment.

@@ -250,6 +242,14 @@ namespace picongpu
static constexpr auto laguerreModes = floatN_X<numModes + 1>(1.0);
static constexpr auto laguerrePhases = floatN_X<numModes + 1>(0.0);
/** @} */

template<typename My = GaussianPulseParam>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically use T_ as default names for template arguments and not My. I would like to stick to that guideline.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already part of the original implementation from @chillenzer.
I agree with you. @psychocoderHPC should this change go in this PR as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed My to T_Self

Copy link
Contributor

@chillenzer chillenzer Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware of this and I didn't stick to that convention because I see it less as a template parameter and more as a hack/workaround for injecting the correct type.

I'm fine with changing it but if so, we should probably do it consistently everywhere and adopt that as the new convention.

@PrometheusPi
Copy link
Member

Interstingly, we get compile errors like:

crp/picongpu/include/picongpu/../picongpu/fields/incidentField/profiles/BaseParam.def:226:63: error: no member named 'TIME_DELAY_SI' in 'picongpu::fields::incidentField::profiles::MyGaussianParam'
  226 |                         result["time_delay"] = {{"value", My::TIME_DELAY_SI}, {"unit", "s"}};

which I did not see when I tested the code on hemera.

"unit": "s",
"value": 5e-15
},
"time_delay": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The time delay was actually outputted correctly for my LWFA test - strange

@@ -223,7 +241,7 @@ namespace picongpu
originToString(My::FOCUS_ORIGIN_X),
originToString(My::FOCUS_ORIGIN_Y),
originToString(My::FOCUS_ORIGIN_Z)}}};
result["time_delay"] = {{"value", My::TIME_DELAY_SI}, {"unit", "s"}};
result["time_delay"] = {{"value", GetTimeDelay<My>()}, {"unit", "s"}};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MUST BE GetTimeDelay<My>::value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - I applied it

@chillenzer
Copy link
Contributor

You've done everything right so far but the CI failures are due to PlaneWaveParam in some examples not inheriting from BaseParam, see e.g. in Bunch and analogously in FoilLCT. This needs to be added and in the best case, we go through the examples and tests by hand and check for such missing inheritance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring code change to improve performance or to unify a concept but does not change public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants