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

Unified testsuite #4334

Merged
merged 2 commits into from
Jun 6, 2023
Merged

Conversation

MSVoss
Copy link

@MSVoss MSVoss commented Oct 28, 2022

This is my suggestion for the generalization of the test suite. As already stated, the current version of the KHI test is very geared towards this. In order to make the test suite usable for different test cases, specific components had to be replaced. Among them are the following aspects in particular:

  • Labeling and description within the results (plot and .log file), including e.g. title, author, ... (see TestKHI.py file, in which these are statically defined)
  • Function for calculating the theoretical values ​​(see TheoryGrowthrate.py)
  • Calculation of the values ​​from the simulation data
  • Limited use of other functions for the general (e.g. function for calculating the deviation)

In order to remedy these shortcomings, the following has been changed (here only the essential aspects):

  • New structure of the test suite for better encapsulation and clarity
  • Introduction of Data.py: This file serves as a parameter file and contains essential parameters of the test, such as author, title, theory calculation function,.. . This eliminates the static character of the old structure.

There are a few things to keep in mind:

  • This is only the first version of the generalization, extensions will follow later
  • Deterioration compared to the previous version: no case distinction, it follows that the test case of the KHI (i.e. MI and ESKHI) currently has to be treated separately
  • Some functions have not yet been completed, such as the 2D plot

Still under development or included in the next "version":

  • Simple case distinction: Using a parameter, the test suite should be able to distinguish individual different theoretical cases from each other (e.g. see KHI: use the Lorentz factor to distinguish whether ESKHI or MI is present)
  • Plotting and evaluating 2D plots
  • Evaluation of openPMD data

Currently integrated tests:

  • MI
  • ESKHI
    Both applicable for 3D as well as for 2D (however, the corresponding .param files have to be adapted to the case). Please note the dependency on the Lorenz factor in 3D. The included ci.bash script uses the MI. For more information, refer to the test case documentation.

Resulting advantage for the user compared to the old version.

  • The result is easily customizable
  • Test developer can freely define functions of the theory
  • ...

Only the most important aspects are mentioned in the list above. Further information can be found in the individual documentation (function documentation)
Documentation will be added later for more information

@MSVoss
Copy link
Author

MSVoss commented Oct 28, 2022

Flake8 will be corrected shortly

@steindev
Copy link
Member

steindev commented Oct 28, 2022

@MSVoss There are about 300 lines of complaints by flake8, this is the reason the pull request validation failed. See report.

EDIT: I did not see your second comment, sorry for bothering you again 🙈

EDIT2: Would you please update the PR description with an explanation of 'generalization'? That is, what is the aim of the generalization and how do you achieve this? (Just a few lines)

@steindev steindev added refactoring code change to improve performance or to unify a concept but does not change public API component: tests unit tests labels Oct 28, 2022
@steindev steindev added this to the 0.7.0 / 1.0.0: Next Stable milestone Oct 28, 2022
s of KHI currently have to be treated differently.
"""
import sys
sys.path.insert(1, "../../")
Copy link
Member

Choose a reason for hiding this comment

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

why do you replace the 2nd entry of the path list?

Copy link
Author

Choose a reason for hiding this comment

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

It's almost random. I could also have used append and simply added it afterwards. But here I'm looking for another solution anyway because flake8 is not fulfilled here.

Copy link
Member

Choose a reason for hiding this comment

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

@MSVoss I think sth like this could possibly also work and would maybe not trigger flake8. But, no idea if this is any better.

from importlib.machinery import SourceFileLoader
testsuite = SourceFileLoader("testsuite", "../../testsuite/__init__.py").load_module()

@MSVoss
Copy link
Author

MSVoss commented Mar 28, 2023

I hope I haven't overlooked anything. I made a note of the inheritance for the next pull request.

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.

Some open questions, but this looks good to me

@PrometheusPi
Copy link
Member

@psychocoderHPC @pordyna You had some open issues and questions on this pull request. What's the status?

@PrometheusPi
Copy link
Member

only open questions by @psychocoderHPC
#4334 (comment)

pordyna
pordyna previously approved these changes Apr 21, 2023
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.

I am blocking merging for now because when executing the CI via

./picongpu/share/picongpu/tests/KHI_growthRate/bin/ci.sh picongpu/share/picongpu/tests/KHI_growthRate/ ./KHI

(on hemera v100) I get the error 42.
Thus, when adding this test, the CI will always fail.

@MSVoss could you clarify what I am doing wrong or what the error 42 is causing.

@PrometheusPi
Copy link
Member

The python code validate.sh throws the following error:

<class 'ValueError'> This should never happen <traceback object at 0x2aab462ff708>

@PrometheusPi
Copy link
Member

@MSVoss the PIP check of our CI is still unhappy with:

./lib/python/test/testsuite/_manager.py:13:1: E302 expected 2 blank lines, found 1

PrometheusPi
PrometheusPi previously approved these changes Jun 2, 2023
@PrometheusPi PrometheusPi added the CI:no-compile CI is skipping compile/runtime tests but runs PICMI tests label Jun 2, 2023
psychocoderHPC
psychocoderHPC previously approved these changes Jun 2, 2023
@@ -27,7 +27,7 @@
#include <pmacc/math/operation.hpp>

#ifndef PARAM_GAMMA
# define PARAM_GAMMA 1.021
# define PARAM_GAMMA 1.2
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 to cmakeFlags

flags[3]="-DPARAM_OVERWRITES:LIST='-DPARAM_GAMMA=1.2'
# Depending on the GPU vendor and version compile can fail because of limited amound of shared memory.
# Please enable the test by hand!
# flags[4]="-DPARAM_OVERWRITES:LIST='-DPARAM_PRECISION=precision64Bit;-DPARAM_PARTICLESHAPE=PQS'"

@@ -27,7 +27,7 @@
#include <pmacc/math/operation.hpp>

#ifndef PARAM_GAMMA
# define PARAM_GAMMA 1.021
# define PARAM_GAMMA 1.2
Copy link
Member

Choose a reason for hiding this comment

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

offline discussed with @PrometheusPi: we should not change the default behavior of this test. We have users which using this test case and we should not silently invalidate the possibility that old and now runs can be compared.


for line in all_Lines:
if "define" + search_u in line and parameter is None:
parameter = float(line[line.find(search_u) +
Copy link
Member

Choose a reason for hiding this comment

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

@MSVoss and @PrometheusPi please discuss this comment

@PrometheusPi PrometheusPi merged commit 7247171 into ComputationalRadiationPhysics:dev Jun 6, 2023
6 checks passed
psychocoderHPC added a commit to psychocoderHPC/picongpu that referenced this pull request Jul 20, 2023
Issue introduced with ComputationalRadiationPhysics#4334

- remove slash at the end of a path (do not enforce a slash)
- add a slash before concadinate a filename with a directory path
psychocoderHPC added a commit to psychocoderHPC/picongpu that referenced this pull request Jul 20, 2023
Issue introduced with ComputationalRadiationPhysics#4334

- remove the slash at the end of a path (do not enforce a slash)
- add a slash before concatenate a filename with a directory path
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI:no-compile CI is skipping compile/runtime tests but runs PICMI tests component: tests unit tests 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.

None yet

6 participants