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

Interpolate jx_beam and jy_beam to level 1 #936

Conversation

AlexanderSinn
Copy link
Member

Interpolate jx_beam and jy_beam to level 1 at the start of the refined patch because they don’t get shifted from the previous slice.

Old:
image
New:
image
Diff:
image

  • 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 component: fields About 3D fields and slices, field solvers etc. mesh refinement anything related to mesh refinement labels May 6, 2023
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.

Great, thanks for this PR! See just 1 comment below. Another possibility would be to actually deposit the beam transverse current on the first slice of the fine grid rather than interpolating it from level 0. Could you elaborate on why you chose to do the interpolation? Just faster and simpler to implement? (This is just for the information, I don't suggest we do the deposition.)

{
if (lev == 0) return; // only interpolate field to lev 1
HIPACE_PROFILE("Fields::LevelUp()");
constexpr int interp_order = 2;
Copy link
Member

Choose a reason for hiding this comment

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

A interp_order stencil was already applied from beam particles to the grid. this is an additional smoothing step that is unnecessary, in particular because we are interpolating to a fine grid to resolve finer structures. A lower interpolation order here should reduce the difference with jx_beam obtained by depositing directly to the fine grid.

Suggested change
constexpr int interp_order = 2;
constexpr int interp_order = 1;

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently all interpolation to level one is done with order two. Should the other functions be changed as well or just this one.

Copy link
Member

Choose a reason for hiding this comment

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

Probably all of them, but we should review this case by case (might sometimes depend on whether it's a source or a field). But you're right, it's not only about this one. Could you open a separate PR setting all others to e.g. 1 (or have a variable controlling them, if that's quick, but we'll probably scan it just in the next few days/weeks before merging the PR and set it to the best value)? Let me know if that works for you, and if so we can merge this PR as is for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is easier to change and test everything in one go after merging this PR with order two.

Copy link
Member

Choose a reason for hiding this comment

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

Let me take it back: we don't need to scan it. I believe that hard-coding it to 1 everywhere is what makes most sense. Then we could just compare old (2) vs. new (1) and see if it's a significant change. But I see no reason how 0 (interpolating from wrong position) or 3 (huge smoothing, as seen from level 1) could ever make sense. So no need to expose it as a variable (unless it's easier in the code).

Copy link
Member

Choose a reason for hiding this comment

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

ok

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me take it back: we don't need to scan it. I believe that hard-coding it to 1 everywhere is what makes most sense. Then we could just compare old (2) vs. new (1) and see if it's a significant change. But I see no reason how 0 (interpolating from wrong position) or 3 (huge smoothing, as seen from level 1) could ever make sense. So no need to expose it as a variable (unless it's easier in the code).

I agree

@AlexanderSinn
Copy link
Member Author

We could deposit the beam for WhichSlice::This but for WhichSlice::Previous1 the beam slice has already been pushed to the next timestep. The deposition could be done one slice in advance but that is significantly more complicated.

{
if (lev == 0) return; // only interpolate field to lev 1
HIPACE_PROFILE("Fields::LevelUp()");
constexpr int interp_order = 2;
Copy link
Member

Choose a reason for hiding this comment

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

ok

@MaxThevenet MaxThevenet merged commit de2affa into Hi-PACE:development May 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: fields About 3D fields and slices, field solvers etc. mesh refinement anything related to mesh refinement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants