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

ABLASTR: Coarsen Functions #3433

Merged
merged 2 commits into from
Dec 8, 2022
Merged

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Sep 28, 2022

Move coarsen functions to ABLASTR.

Needed for BLAST-ImpactX/impactx#261

Follow-up to:

Edit by @EZoni:
Here's a few slides describing the interpolation operations refactored in this PR (interpolation between grids with arbitrary index type and mesh refinement ratio, for I/O and MR): Slides-Interpolation-IO-MR.pdf. The slides date back to 2020, when the PRs listed above were opened and merged.

@ax3l ax3l added component: mesh refinement (A)MR component: ABLASTR components shared with other PIC codes labels Sep 28, 2022
@ax3l ax3l requested review from RemiLehe and EZoni September 28, 2022 22:14
@ax3l ax3l mentioned this pull request Sep 28, 2022
6 tasks
@ax3l ax3l force-pushed the ablastr-coarsen branch 2 times, most recently from 0426bb3 to 8990c9a Compare October 28, 2022 20:58
@ax3l ax3l marked this pull request as ready for review October 28, 2022 20:58
@ax3l ax3l changed the title [WIP] ABLASTR: Coarsen Functions ABLASTR: Coarsen Functions Oct 28, 2022
@ax3l ax3l force-pushed the ablastr-coarsen branch 3 times, most recently from a82dd27 to d439916 Compare October 28, 2022 21:06
@@ -70,19 +88,18 @@ namespace CoarsenIO{
const int jmin = idx_min[1];
const int kmin = idx_min[2];
int ii, jj, kk;
Real wx, wy, wz;
amrex::Real const wx = 1.0_rt / static_cast<amrex::Real>(numx);
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 moved this out as a tiny optimization. The compiler probably did this already.

@EZoni
Copy link
Member

EZoni commented Oct 31, 2022

@ax3l I added some comments above, which hopefully clarify the scope of the various interpolation functions that you are refactoring here, to the best of my knowledge. As a general remark, note that the two sets of interpolation routines had been named originally with the tags IO and MR, because the type of interpolation we chose to do for I/O and MR is different (simple nearest-neighbors interpolation for I/O, and more complicated weights to guarantee certain conservation properties for MR - more details in the slides linked in the PR description), regardless of whether there could be constraints on the index type of the input/output data in certain cases.

@ax3l
Copy link
Member Author

ax3l commented Nov 1, 2022

Thanks @EZoni - I think all that is left then is finding a good name, especially for the I/O stuff, which I find not very descriptive. Do you think we could describe the properties of the two classes (e.g., conservation properties and time/space average) in the name(s)?

@EZoni
Copy link
Member

EZoni commented Nov 2, 2022

@ax3l We discussed with @jlvay and @WeiqunZhang and had the following ideas:

  • The old CoarsenIO operation could be renamed sample;
  • The old CoarsenMR operation could be renamed average (as in the AMReX AverageDown) or interpolate.

Let's discuss here more if you have any comments or concerns on these suggestions.

P.S. Again, if we want the I/O one to be specialized for cell-centered output, there is the option of simplifying the formulas, by setting the corresponding output index type to zero in the code, and then calling it sample_to_cell_centers or something along these lines.

@ax3l
Copy link
Member Author

ax3l commented Nov 3, 2022

Thanks a lot - I updated the PR accordingly and it's now ready for review with the new names :)

@EZoni
Copy link
Member

EZoni commented Nov 4, 2022

Copying here what I commented on Slack.

Would it be possible and would it make sense to consider removing one intermediate layer? Like doing something like this:

  • rename namespace ablastr::coarsen::sample as namespace ablastr:sample_with_coarsening
  • rename namespace ablastr::coarsen::average as namespace ablastr::average_with_coarsening

(or similar names), which keeps the information about "coarsening" in the name but removes one layer in terms of structure?

Then one would have things like
ablastr::sample_with_coarsening::Coarsen or ablastr::sample_with_coarsening::Interp
instead of things like (current code)
ablastr::coarsen::sample::Coarsen or ablastr::coarsen::sample::Interp.

What do you think?

@ax3l
Copy link
Member Author

ax3l commented Dec 6, 2022

Thanks a ton! Discussed between Edoardo, Remi & Axel: we'll keep the current naming for now since it mirrors the directory structure.
I'll rebase. - Done.

Move coarsen functions to ABLASTR.
Rename by property of the coarsening function.
Copy link
Member

@EZoni EZoni left a comment

Choose a reason for hiding this comment

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

Thank you, @ax3l! This looks great. I just left two minor comments, see below.

Co-authored-by: Edoardo Zoni <[email protected]>
Copy link
Member

@EZoni EZoni left a comment

Choose a reason for hiding this comment

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

Great, ready to merge! I think I'll open an issue to keep track of the fact that it would be good to add some documentation of these interpolation methods (sampling, averaging), including a link to the slides attached in the PR description, as you suggested some time ago.

@ax3l
Copy link
Member Author

ax3l commented Dec 8, 2022

Good idea! You could copy the slide content in a new theory sub-section of the manual.

@EZoni EZoni merged commit 202b93c into ECP-WarpX:development Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ABLASTR components shared with other PIC codes component: mesh refinement (A)MR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants