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 openPMD-viewer test #4747

Conversation

max-lehman14
Copy link
Contributor

A simple example is used and the input parameters are compared to the readout of the openPMD-viewer. This test involves the electromagnetic fields, electron and ion charge density, particle positions and momenta.

@psychocoderHPC psychocoderHPC added component: core in PIConGPU (core application) component: tests unit tests labels Nov 21, 2023
@psychocoderHPC psychocoderHPC added this to the 0.8.0 / Next stable milestone Nov 21, 2023
@psychocoderHPC
Copy link
Member

Please reformat your python files to be flake8 conform.

@max-lehman14
Copy link
Contributor Author

@PrometheusPi I am using openpmd_viewer 1.7.0

@PrometheusPi
Copy link
Member

Please add a requirements.txt

@PrometheusPi PrometheusPi marked this pull request as ready for review December 14, 2023 15:06
@PrometheusPi
Copy link
Member

I tested the new test with

./picongpu/share/picongpu/tests/openPMD-viewer-test/bin/ci.sh picongpu/share/picongpu/tests/openPMD-viewer-test/ ./run01

on hemera and it works fine.
Will do a proper code review asap.

Copy link
Member

@PrometheusPi PrometheusPi left a comment

Choose a reason for hiding this comment

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

There are a few (minor) code changes - see comments.

I tested the code to fail by changing the PIConGPU param files to disagree with the (python) parameter dictionary against it was compared.
That lead to the intended errors when:

  • changing the background electric field
  • changing the gamma from 1.021 to 2.021
  • not adding ion species

When rebasing against the mainline dev, I assume one gets compile time errors.
(I currently could not test that due to high work load on hemera's v100)
I will test it myself asap, but_
--> Please rebase against the current dev

share/picongpu/tests/openPMD-viewer-test/bin/ci.sh Outdated Show resolved Hide resolved
share/picongpu/tests/openPMD-viewer-test/README.rst Outdated Show resolved Hide resolved
share/picongpu/tests/openPMD-viewer-test/bin/ci.sh Outdated Show resolved Hide resolved
share/picongpu/tests/openPMD-viewer-test/bin/ci.sh Outdated Show resolved Hide resolved
Comment on lines 130 to 132
eps_s = 1
eps_l = np.abs(
field_param[f"{density}"] * 1e-1
Copy link
Member

Choose a reason for hiding this comment

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

see comment before: please define this in your parameters dictionary (or a separate epsilon dictionary)

Copy link
Member

Choose a reason for hiding this comment

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

what is the meaning of eps_s and eps_l please add a comment

Copy link
Member

Choose a reason for hiding this comment

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

                field_param[f"{density}"] * 1e-1

Why do we accept an error of 10%? Is this due to the particle motion or can this be reduced?

]
for item in array
]
eps = 0.1
Copy link
Member

Choose a reason for hiding this comment

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

see comment above on epsilons


bool_pass = check_params(2) # false if failure
print("Test result: ", bool_pass)
sys.exit(not bool_pass)
Copy link
Member

Choose a reason for hiding this comment

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

please add a comment on why we needed to negate the return value - otherwise this is really confusing


This test reads out openPMD-viewer data and checks if it matches the input values to see if users can rely on the output.

To run this test, one has to execute ci.sh with the location of the input and output directory.
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a codeblock to show that?

@PrometheusPi
Copy link
Member

I rebased against the current dev. There is no compile isssue - thus density.param is still fine.
Thus, @max-lehman14 please just a code commented above.

@PrometheusPi
Copy link
Member

@max-lehman14 There are still compile errors in your code ee.g.:

error: 'AssignXDriftPositive' is not a member of 'picongpu::particles::manipulators';

@steindev steindev added the changelog PR's marked with this label will be added to the changelog label Jan 19, 2024
@BrianMarre
Copy link
Member

BrianMarre commented Apr 24, 2024

@max-lehman14 the CI fails due to an undocumented change in behaviour in recent typeguard versions first noted in issue #4853 and fixed by PR #4861, please rebase to the current dev

Copy link
Member

@PrometheusPi PrometheusPi left a comment

Choose a reason for hiding this comment

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

Looks very good @max-lehman14 just a few minor issues left.

@PrometheusPi
Copy link
Member

@max-lehman14 I think your code base is too old and thus triggers a CI error that your actual code does not cause.
Please update the code of your pull request via: git pull --rebase mainline dev and then push the branch to GitHub. We can do this together tomorrow of needed.

@PrometheusPi
Copy link
Member

@max-lehman14 perfect - could you please squash all your commits to one.

@PrometheusPi PrometheusPi merged commit b23d490 into ComputationalRadiationPhysics:dev May 29, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog PR's marked with this label will be added to the changelog component: core in PIConGPU (core application) component: tests unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants