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

Readin from HiPACE-C files improved + conversion script #315

Merged
merged 4 commits into from
Jan 19, 2021

Conversation

AlexanderSinn
Copy link
Member

@AlexanderSinn AlexanderSinn commented Jan 13, 2021

This PR introduces the necessary files to read in a beam from file from HiPACE-C, as well as the ability to read in beams from file for a given charge and plasma density, which was a commonly used feature in HiPACE-C for experimental simulations.

Check that all corner cases work:

  • 1. read in from HiPACE-C file (running in normalized units)
  • 2. read in from HiPACE-C file (running in SI units)
  • 3. read in from file generation script (running in normalized units)
  • 4. read in from file generation script (running in SI units)
  • 5. read in from file with beam charge and plasma density (running in normalized units)
  • 6. read in from file with beam charge and plasma density (running in SI units)
  1. Convert the raw beam data files dumped from HiPACE-C via
    python3 convert_hipace_to_hipace++_file.py raw_beam_file.h5. They need to have raw in their name.

For Hipace++ the beam.plasma_density parameter now doesn't need to be set if using a beam from either write_beam.py or convert_hipace_to_hipace++_file.py as it is automatically transferred via an attribute, however it can be overwritten.

  • Small enough (< few 100s of lines), otherwise it should probably be split into smaller PRs
  • Tested (describe the tests in the PR description)
  • Runs on GPU (basic: the code compiles and run well with the new module)
  • Contains an automated test (checksum and/or comparison with theory)
  • Documented: all elements (classes and their members, functions, namespaces, etc.) are documented
  • Constified (All that can be const is const)
  • Code is clean (no unwanted comments, )
  • Style and code conventions are respected at the bottom of https://github.com/Hi-PACE/hipace
  • Proper label and GitHub project, if applicable

@AlexanderSinn AlexanderSinn changed the title [WIP] Readin from HiPACE-C files improved + conversion scriptProduction branch 2 [WIP] Readin from HiPACE-C files improved + conversion script Jan 13, 2021
@AlexanderSinn AlexanderSinn added the component: beam About the beam species label Jan 13, 2021
@AlexanderSinn AlexanderSinn changed the title [WIP] Readin from HiPACE-C files improved + conversion script Readin from HiPACE-C files improved + conversion script Jan 13, 2021
Copy link
Member

@MaxThevenet MaxThevenet left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for this PR! As discussed offline with @SeverinDiederichs let us not put the conversion Python script on the repo for now.

@ax3l
Copy link
Member

ax3l commented Jan 15, 2021

Just curious: why not add the script? Looks useful :)

@MaxThevenet
Copy link
Member

Yes, actually I am wondering now. I just have 2 elements in that direction:

  1. @SeverinDiederichs do you think other Hipace users would benefit from it? If Severin is the only user, I think it can stay out of the repo (our intention was to share it on another platform like Confluence). After all, Hipace was not an open-source code, it does not have that large a user base.
  2. Although it has not even started, I think a library to generate and convert particle distributions --between different geometries, file formats, types (particle distribution vs. density maps), etc. -- would be useful. This script would naturally fit into such a library.

But I don't have a strong opinion, sharing it on the repo just makes it easy to refer to, so that would be fine for me too (after adding a bit of documentation).

Any opinion @SeverinDiederichs @ax3l @AlexanderSinn?

@ax3l
Copy link
Member

ax3l commented Jan 15, 2021

I think we can ship it, it's not much "bloat" and if we don't need it anymore or refactor it in a lib we can always remove it later.

@MaxThevenet
Copy link
Member

MaxThevenet commented Jan 15, 2021

Alright, let's keep it then. @AlexanderSinn @SeverinDiederichs could you then write a docstring at the beginning of the file, explain its capabilities, how to use it, the options, etc.?

@SeverinDiederichs
Copy link
Member

Ok, if we keep the file, I have to request further changes:
@AlexanderSinn

Currently, the convert script only works, if a density is provided. However, for the case, where I want to use the HiPACE-C output file and convert it into a HiPACE++ file, where I would like to run in normalized units, too, I don't want to specify a density (as there was no original density). For this, the unitSI should be 1. You can test this with the HiPACE-C output file, which should be converted without specifying the density, and then running in normalized units in HiPACE++. (to run in SI units, one would need to specify the density).

@MaxThevenet MaxThevenet requested review from SeverinDiederichs and removed request for MaxThevenet January 19, 2021 17:30
Copy link
Member

@SeverinDiederichs SeverinDiederichs left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks!
I will use the new features in production runs right away

@SeverinDiederichs SeverinDiederichs merged commit 039b110 into development Jan 19, 2021
@SeverinDiederichs SeverinDiederichs deleted the production_branch_2 branch January 19, 2021 21:11
@ax3l
Copy link
Member

ax3l commented Jan 20, 2021

HI @AlexanderSinn, thank you again for the great contributions to HiPACE++! 🚀 ✨

Just saw this in your commits:
In order to make sure we can properly attribute your contributions, please go to https://github.com/settings/emails and add the e-mails you prefer for your science/open source work. Confirm the e-mails that you get when setting those please. Those e-mails will be associated with your account and commits on the web GUI of GitHub.

Please set the same primary e-mail on your local workstations from which you git commit your work. This can be done as follows on the command line:

git config --global user.name NAME
git config --global user.email EMAIL@EXAMPLE.com

Use the same NAME spelling as on your GitHub profile: https://github.com/settings/profile

Thank you! ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: beam About the beam species
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants