Skip to content

Enable evolutions using non-conforming Blocks.#7202

Open
kidder wants to merge 3 commits into
sxs-collaboration:developfrom
kidder:evolution_actions_nonconforming
Open

Enable evolutions using non-conforming Blocks.#7202
kidder wants to merge 3 commits into
sxs-collaboration:developfrom
kidder:evolution_actions_nonconforming

Conversation

@kidder
Copy link
Copy Markdown
Member

@kidder kidder commented Apr 18, 2026

Proposed changes

Enables evolutions with spherical shells next to cubed spheres. Tested with the input file for scalar wave.

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

  • Updated tests for the evolution actions are in progress; others want to experiment with running before the tests are finished.
  • Will fail if the spherical shell dynamically p-refines (will be fixed with PR updating the ApplyBoundaryCorrections test)
  • Will fail if LTS chooses different time steps on the two sides of the interface. Fix is to use the FixedLtsRatio in these Elements. This ratio needs to be specified in the input file, probably in the Evolution group.

Copy link
Copy Markdown
Member

@nilsdeppe nilsdeppe left a comment

Choose a reason for hiding this comment

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

Overall looks good. A few suggestions.

@wthrowe could you take a look too?

*all_mortar_data.at(mortar_id).local().mortar_data.value();
// neighbor_boundary_data_on_mortar =
// interpolator.interpolate_to_neighbor(host_data);
interpolated_boundary_data = InterpolatedBoundaryData<Dim>{
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.

remove?

// If we only have one conforming neighbor in this direction, we may or
// may not have to do any projection. If we don't have to do projection,
// then we can use the local_mortar_data itself to calculate the
// dg_package_data. However, if we need to project, then we hae to use
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.

typo we hae to -> we have to

// the packaged_data_buffer that was passed in.
if (neighbors_in_direction.are_conforming() and
Spectral::needs_projection(face_mesh, mortar_mesh, mortar_size)) {
// Have to use packaged_data_buffer
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.

remove?

const auto& element = db::get<domain::Tags::Element<volume_dim>>(*box);
const auto expected_messages = static_cast<size_t>(alg::accumulate(
mortar_next_time_step_ids, 0,
[&mortar_infos, &element, &time_to_process](size_t total,
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.

const size_t total?

static_cast<ptrdiff_t>(
volume_mesh.slice_away(direction.dimension())
.number_of_grid_points()),
[](double v) { return std::isnan(v); }),
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.

const double. Have you tested that this doesn't cause an FPE? There are sometimes weird things with signaling_NaNs...

volume_mesh.slice_away(direction.dimension())
.number_of_grid_points()),
[](double v) { return std::isnan(v); }),
"Not all points were interpolated");
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.

It would probably be helpful to print out the current ElementId, neighbor ID, etc.

Copy link
Copy Markdown
Member

@wthrowe wthrowe left a comment

Choose a reason for hiding this comment

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

The logic for receiving data looks right to me. My main concern is it looks like this introduces another fixed-size-map overflow. Have you tested with a sphere with more than 24 neighbors? If I'm interpreting it correctly, the test input file has exactly 24.

}
neighbor_meshes) {
neighbor_meshes->insert_or_assign(received_mortar_id,
neighbor_mesh);
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.

Doesn't this have the same "more than 24 neighbors" problem we had with the inbox?

From a quick grep of the code, we might only need the neighbor meshes for subcell these days, but I'm not certain. So it may be valid to just not record it near spheres.

const auto& interpolator =
mortar_info.at(mortar_id).interpolator().value();
const auto& host_data =
*all_mortar_data.at(mortar_id).local().mortar_data.value();
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.

This is the same as neighbor_boundary_data_on_mortar set a few lines above.

@nikwit
Copy link
Copy Markdown
Contributor

nikwit commented Apr 24, 2026

when trying out this PR combined with #7205 for a bbh evolution, I do get an error which looks like what Will pointed out above:

std::pair<FixedHashMapIterator<MaxSize, Key, std::pair<const Key, ValueType>, Hash, KeyEqual>, bool> FixedHashMap<MaxSize, Key, ValueType, Hash, KeyEqual>::insert_or_assign_impl(key_type&&, M&&) [with bool Assign = false; M = Mesh<3>; long unsigned int MaxSize = 24; Key = DirectionalId<3>; ValueType [...] ::hash<DirectionalId<3> >; KeyEqual = std::equal_to<DirectionalId<3> >; key_type = DirectionalId<3>] in /home/nwittek/spectre/src/DataStructures/FixedHashMap.hpp:462

Unable to insert element into FixedHashMap of maximum size 24 because it is full. If the current size () is not equal to the maximum size then please file an issue with the code you used to produce this error.

@macedo22
Copy link
Copy Markdown
Contributor

I happened to do a similar experiment to Niko without knowing and came here to also say that I tried combining this PR with #7205 and ran ExportCoordinates3D just to make sure the domain looked right with and without using spherical shells for the outer shell of BCO and I also got the FixedHashMap error. Here's my branch. The first commit just changes CBCO with changes similar to #7205 and I get the same FixedHashMap error when I try the spherical shell there, too

@kidder kidder force-pushed the evolution_actions_nonconforming branch from a1ccb81 to ac77242 Compare April 26, 2026 01:28
@kidder
Copy link
Copy Markdown
Member Author

kidder commented Apr 26, 2026

  • @nikwit @macedo22 I added a fixup commit that should prevent the FixedHashMap error if you want to try it out
  • I have a complete working test for the ComputeTimeDerivative action now; still need one for the ApplyBoundaryCorrections action

@kidder kidder force-pushed the evolution_actions_nonconforming branch from ac77242 to 81c2f96 Compare April 28, 2026 15:53
@nikwit
Copy link
Copy Markdown
Contributor

nikwit commented Apr 28, 2026

The fixup does solve the problem with the map size. The evolution starts now but I am getting a deadlock at ~0.1M but I am not sure yet where that is coming from.

@kidder
Copy link
Copy Markdown
Member Author

kidder commented Apr 28, 2026

The fixup does solve the problem with the map size. The evolution starts now but I am getting a deadlock at ~0.1M but I am not sure yet where that is coming from.

If you are using local time-stepping, then it could be that there is a mismatch in the time step on the spheres vs the wedges as there is nothing in the code yet to enforce them to be equal. There is a mechanism to enforce the time steps to be the same for elements next to a DG-subcell boundary, so it will be straightforward to add something similar.

@nikwit
Copy link
Copy Markdown
Contributor

nikwit commented Apr 28, 2026

Is it necessary for the time steps to be the same on the spherical shell and the Wedges it neighbors?

@kidder
Copy link
Copy Markdown
Member Author

kidder commented Apr 29, 2026

yes

@nikwit
Copy link
Copy Markdown
Contributor

nikwit commented Apr 29, 2026

I fixed the time step the same way subcell does it. This seems to have fixed the deadlock. However, the affected elements are stuck with the initial, very small time step which slows down the simulation massively so we should probably come up with something better than that.

@wthrowe
Copy link
Copy Markdown
Member

wthrowe commented May 6, 2026

The fixups look fine to me. As noted, there are LTS issues that have to be worked out, but I don't think that needs to block this PR.

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.

5 participants