Skip to content

Zernikeb1 subcell projection#7241

Open
michaeldmurphy1 wants to merge 3 commits into
sxs-collaboration:developfrom
michaeldmurphy1:zernikeb1_subcell_projection
Open

Zernikeb1 subcell projection#7241
michaeldmurphy1 wants to merge 3 commits into
sxs-collaboration:developfrom
michaeldmurphy1:zernikeb1_subcell_projection

Conversation

@michaeldmurphy1

@michaeldmurphy1 michaeldmurphy1 commented May 15, 2026

Copy link
Copy Markdown
Contributor

Proposed changes

We need ZernikeB1 projection for cartoon subcell. We need to know the parity for each projection, which can either be passed or inferred by the type (so now the type needs to be passed).

This is achieved by either passing the parity as an argument for DataVectors, or for when the passed DataVector is multiple components (i.e. GhostData) the underlying types must be passed.

Depends on:

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.
  • If a coding agent is used, have one of
    "Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com",
    "Co-Authored-by: Codex noreply@openai.com", or
    "Co-Authored-By: GitHub Copilot CLI noreply@microsoft.com"
    as the last line of the commit, depending on the agent.

Further comments

@michaeldmurphy1 michaeldmurphy1 force-pushed the zernikeb1_subcell_projection branch 2 times, most recently from 0581a24 to a6e10d1 Compare May 15, 2026 17:42
michaeldmurphy1 and others added 3 commits May 31, 2026 21:09
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ZernikeB1 bases, used with Cartoon, require the parity of the component to be
known as the interpolation requires this to be sufficiently accurate. When the
projection is of a DataVector, it must either be a single component with the
parity passed as an argument, or the underlying tmpl::list to types that make up
the DataVector must be passed as a template.

The GrMhd systems are updated to correctly call the projection, while all other
systems are simply updated to work with the new interface.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@michaeldmurphy1 michaeldmurphy1 force-pushed the zernikeb1_subcell_projection branch from a6e10d1 to 946005e Compare June 1, 2026 01:09
#include "Evolution/DgSubcell/CombineVolumeGhostData.hpp"
#include "Evolution/DgSubcell/GhostData.hpp"
#include "Evolution/DgSubcell/NeighborRdmpAndVolumeData.hpp"
#include "Evolution/DgSubcell/NeighborRdmpAndVolumeData.tpp"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Including this in general headers mostly defeats the purpose of having a tpp file. Would explicitly instantiating the functions be feasible?

Alternatively, I might have missed something but I don't see anything obvious that requires the parity information to be constexpr, so you could make the function in the hpp something that calls compute_parity_list and passes the information to an impl that doesn't have to be templated on that tags. (The array could be converted to an initializer_list, which though sometimes painful to work with isn't templated on the size.)

* \brief Computes the projection matrix in 1 dimension going from a DG
* mesh to a conservative finite difference subcell mesh.
*
* The parity parameter is required for ZernikeB1 bases.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"and is ignored for others" or something similar.

const DirectionalIdMap<Dim, std::optional<intrp::Irregular<Dim>>>&
Neighbor_dg_to_fd_interpolants);
neighbor_dg_to_fd_interpolants,
tmpl::list<VolumeFields> /*meta*/);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this a list<list<tags...>>? Do you need the second list?

Comment on lines +138 to +141
DataVector even_input(num_even * num_dg_pts, 0.0);
DataVector odd_input(num_odd * num_dg_pts, 0.0);
DataVector even_output(num_even * num_ghost_pts_per_var, 0.0);
DataVector odd_output(num_odd * num_ghost_pts_per_var, 0.0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do these need to be initialized to 0, or can you leave them uninintialized?

Comment on lines +138 to +141
DataVector even_input(num_even * num_dg_pts, 0.0);
DataVector odd_input(num_odd * num_dg_pts, 0.0);
DataVector even_output(num_even * num_ghost_pts_per_var, 0.0);
DataVector odd_output(num_odd * num_ghost_pts_per_var, 0.0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A possible optimization would be to allocate these as a single buffer.

const DirectionalIdMap<Dim, std::optional<intrp::Irregular<Dim>>>&
neighbor_dg_to_fd_interpolants) {
neighbor_dg_to_fd_interpolants,
tmpl::list<VolumeFields> meta) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you're actually going to use the variable, name it something more descriptive than "meta".

template <typename TagList, size_t Dim>
void project(const gsl::not_null<DataVector*> subcell_u, const DataVector& dg_u,
const Mesh<Dim>& dg_mesh, const Index<Dim>& subcell_extents,
tmpl::list<TagList> /*meta*/) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same question as above about lists of lists.

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