Skip to content

A rework for the advection module that improves WENO performance #4434

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

Open
wants to merge 232 commits into
base: main
Choose a base branch
from

Conversation

simone-silvestri
Copy link
Collaborator

@simone-silvestri simone-silvestri commented Apr 22, 2025

This PR reworks how advection is implemented by passing a reduced_order integer to the interpolate functions.

At the moment, upwind and centered reconstruction do not formally change; the ifelse that was upstream before in the alt_interpolate functions is just pushed downstream in the reconstruction function.

For WENO, the reconstruction approach changes because the order is accounted for by changing the reconstruction coefficients and the smoothness coefficients.
This method should give us a boost in performance in the WENO case for bounded and immersed boundary grids, as there is much less computation to be performed. I will post some benchmarking later on.

This PR is still exploratory, so there is a bit of benchmarking and cleaning up to do.

EDIT: I think this PR might be ready

  • PRO: faster code with a computational saving that might scale much more efficiently for larger kernels
  • CON1: more difficult to mix and match different schemes together (for example, having WENO that falls back to Centered near the boundaries)
  • CON2: very difficult to implement higher orders (in main it is sufficient to add a number to the buffers and add the specific coefficients, here you would have to add also the coefficients for the intra-order reconstructions). I am not sure how important this CON is since I don't envision using advection orders higher than 10 for centered and 9 for upwind schemes

Removes unused features:

  • orders 12 for centered and 11 for upwind (never used, unstable, and just a waste of space)
  • vestigial code for stretched advection (now we only have constant coefficients, but some code to enable stretched advection remained after Remove stretched advection #4411)

@simone-silvestri
Copy link
Collaborator Author

simone-silvestri commented Aug 13, 2025

I will fix the tests then I think this PR might be ready for review:

  • PRO: faster code with a computational saving that might scale much more efficiently for larger kernels
  • CON1: more difficult to mix and match different schemes together (for example having WENO that falls back to Centered near the boudaries)
  • CON2: very difficult to implement higher orders (in main you just add a number to the buffers, here you would have to add also the coefficients). I am not sure how important this CON is since I don't envision using advection orders higher that 10 for centered and 9 for upwind schemes

Removes unused features:

  • orders 12 for centered and 11 for upwind (never used, unstable, and just a waste of space)
  • vestigial code for stretched advection (now we only have constant coefficients but some code to enable stretched advection remained after Remove stretched advection #4411)

EDIT: I will add this to the description of the PR so that it does not get lost in the wall of commits....

@simone-silvestri simone-silvestri marked this pull request as ready for review August 14, 2025 07:29
@simone-silvestri simone-silvestri changed the title A rework for the advection module that (should) improve WENO performance A rework for the advection module that improves WENO performance Aug 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark performance runs preconfigured benchamarks and spits out timing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants