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

Lagrange3oprolong #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Lagrange3oprolong #2

wants to merge 2 commits into from

Conversation

rhaas80
Copy link
Member

@rhaas80 rhaas80 commented Aug 8, 2024

@zachetienne pull request 21 from https://bitbucket.org/eschnett/carpet/pull-requests/21

When the default prolongation type is chosen in a dynamical spacetime evolution, hydrodynamic and GRMHD fields can suffer from spurious oscillations generated at AMR refinement boundaries due to the high-order Lagrange polynomial prolongation.

This patch adds a simple Lagrange third-order prolongation operator to Carpet, which is more suitable for hydro and GRMHD field prolongations.

I would like for this to be the default prolongation type for the evolved conservative variables in IllinoisGRMHD and GiRaFFE, starting in the next ETK release (2018 H2), as it reduces numerical noise at AMR boundaries.

@rhaas80
Copy link
Member Author

rhaas80 commented Aug 8, 2024

@zachetienne says

I created a trac ticket. Agreed that it may be a bit hacky, but the alternative would require some doing, and this would be immediately useful as I plan to make it the default prolongation option for IllinoisGRMHD and GiRaFFE evolved variables.

@rhaas80
Copy link
Member Author

rhaas80 commented Aug 8, 2024

If you want a review for the ET, would you mind creating a ticket on the ET trac and linking the ticket and the pull request, please? Not having looked at anything yet: Carpet does provide lower order Lagrange interpolation operators, should you not be able to use those?

not having looked

you are doing this.

….

Seeing the number of hacks we now have for hydro ops. it may be useful to somehow give the ops a way of having a per-variable order so that one does not have to invent a new name each time one needs a new order. If needs be via a ProlongationOperatorOptsName tag.

T const *restrict const src, ivect3 const &restrict srcpadext,
ivect3 const &restrict srcext, T *restrict const dst,
ivect3 const &restrict dstpadext, ivect3 const &restrict dstext,
ibbox3 const &restrict srcbbox, ibbox3 const &restrict dstbbox,
Copy link
Member Author

Choose a reason for hiding this comment

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

@eschnett says

This chooses third order interpolation even if the overall order is set to be first order. This will bypass the check for sufficient number of ghost zones. The operator’s check will still catch this, but with an error message that is difficult to interpret.

I suggest changing this, so that the the_operators array is set up to contain the actual desired operators, and then using the respective array element here.

Otherwise there is no point in setting up an array, and you can simple pass &prolongate_3d_rf2<T, 3> as argument here.

@rhaas80
Copy link
Member Author

rhaas80 commented Aug 8, 2024

For reference, the ticket is https://bitbucket.org/einsteintoolkit/tickets/issues/2189

@rhaas80
Copy link
Member Author

rhaas80 commented Aug 8, 2024

I am still not really happy about having new operators defined when really all one wants is to decouple the number of ghost points required from the operator order setting for the Lagrange operators. In particular if the only reason to introduce it is to avoid triggering a consistency check between ghost zone width and operator order.

Instead of introducing a new operator I would much prefer if Carpet was to take all operators requested by all grid functions and takes the maximum number of ghosts required by these as the thing to check. While a grid function may have a operator assigned to it and never have SYNC called, the operator will still be used after regridding to fill in new grid points on a fine grid, so that there need to be enough ghost zones to satisfy all operators associated with grid functions that have storage (possibly restricted to those that have more than one time level but I am not sure about that and would have to check Carpet’s refinement prolongation code).

Copy link
Member Author

@rhaas80 rhaas80 left a comment

Choose a reason for hiding this comment

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

transferred comments from bitbucket

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants