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

Per-slice fixed_ppc beam initialization #1000

Conversation

AlexanderSinn
Copy link
Member

  • 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 added the component: beam About the beam species label Jul 27, 2023
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.

Great work! See a few minor comments below, hope we can merge this soon.

Comment on lines 237 to 238
/** Width of the Gaussian beam. Only used for a fixed-weight beam */
amrex::RealVect m_position_std {0., 0., 0.};
Copy link
Member

Choose a reason for hiding this comment

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

This has nothing to do with this PR, but isn't that also used for a fixed ppc beam with a Gaussian profile?

Copy link
Member Author

Choose a reason for hiding this comment

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

Position std for the fixed ppc Gaussian profile is stored inside GetInitialDensity, so that variable in BeamParticleContainer is only used for fixed weight beams.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, that makes sense

@@ -202,7 +199,7 @@ BeamParticleContainer::InitData (const amrex::Geometry& geom)
amrex::ParallelDescriptor::Communicator());
#endif

if (Hipace::HeadRank()) {
if (Hipace::HeadRank() && m_injection_type != "fixed_ppc") {
m_init_sorter.sortParticlesByBox(*this, geom);
}
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 we still sort particles by box when we removed the loop over boxes with the per slice comms?

Copy link
Member Author

Choose a reason for hiding this comment

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

The function actually sorts per slice (1 box == 1 slice there)

Comment on lines -80 to -83
if ( ncells_total / Hipace::m_beam_injection_cr / Hipace::m_beam_injection_cr
> std::numeric_limits<int>::max() / 100 ){
amrex::Print()<<"WARNING: the number of cells is close to overflowing the maximum int,\n";
amrex::Print()<<"consider using a larger hipace.beam_injection_cr\n";
Copy link
Member

Choose a reason for hiding this comment

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

With the per slice init, the m_beam_injection_cr is deprecated. Could you please fully remove it from the code and add it to the deprecated input paramters?

Copy link
Member Author

Choose a reason for hiding this comment

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

I fully removed it

Comment on lines +283 to +299

rarrdata[BeamIdx::x ][pidx] = x;
rarrdata[BeamIdx::y ][pidx] = y;
rarrdata[BeamIdx::z ][pidx] = z;
rarrdata[BeamIdx::ux ][pidx] = u[0] * speed_of_light;
rarrdata[BeamIdx::uy ][pidx] = u[1] * speed_of_light;
rarrdata[BeamIdx::uz ][pidx] = u[2] * speed_of_light;
rarrdata[BeamIdx::w ][pidx] = std::abs(weight);

// ensure id is always valid
// it will repeat if there are more than 2^31-1 particles
int id = int((pid + pidx) & ((1ull << 31) - 1));
if (id == 0) id = 1;

iarrdata[BeamIdx::id ][pidx] = id;
iarrdata[BeamIdx::cpu][pidx] = 0;

Copy link
Member

Choose a reason for hiding this comment

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

Why can't this be in AddOneBeamParticle? The extra safety for more than 2^31-1 particles should also become relevant for fixed weight beams, so it would be nice to add this to the function and call the function. Or am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

AddOneBeamParticle does not initialize BeamIdx::cpu and takes slightly different function arguments than what are available here.

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.

Thanks for this PR! LGTM

@AlexanderSinn AlexanderSinn merged commit fb3f621 into Hi-PACE:development Aug 9, 2023
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.

2 participants