Skip to content

Add unit tests for ShiftedMetricInterp#3262

Merged
bendudson merged 4 commits into
nextfrom
unit-test-shiftedmetricinterp-debug
Feb 6, 2026
Merged

Add unit tests for ShiftedMetricInterp#3262
bendudson merged 4 commits into
nextfrom
unit-test-shiftedmetricinterp-debug

Conversation

@ZedThree
Copy link
Copy Markdown
Member

Because this method should be identical to ShiftedMetric, we can just parameterise the existing unit tests over the two types (although the FieldPerp overloads are not implemented so we have to skip those). This is in support of the work to parallelise in Z, by getting more tests on bits that that touches.

One important thing to note is that ZHermiteSpline (the z-only interpolation) explicitly doesn't do anything in any of the guards. This means that f.yup(mesh->yend + 1) does not get filled in. There's a comment the guards should be filled in by the boundary conditions, but I'm not actually sure that's true for the parallel slices.

In the second commit, I end up just masking the guards out basically, just to make the test pass.

If I instead comment out the region masking in ZInterpolation, and just use RGN_NOY instead, all the tests seem to pass, including the integrated tests that use MAST grids.

@dschwoerer @johnomotani Thoughts? Am I missing something why we can't just use RGN_NOY for the Z interpolation here?

@johnomotani
Copy link
Copy Markdown
Contributor

Not sure what the right thing to do is. It seems like I got "RGN_NOBNDRY" (region = localmesh->getRegion3D("RGN_NOBNDRY");), then tried to remove the boundary points at the y-boundaries, but surely they were not included already? Maybe I have just forgotten what "RGN_NOBNDRY" includes.

I think my expectation when writing this was that if using parallel slices, y boundary conditions would always be applied directly to the parallel slices, but I think that's not necessarily the case (e.g. boundary conditions that are applied by transforming to/from field-aligned)? So sometimes the interpolation should be done in the y-boundaries? My concern would be that in cases where the boundary condition is directly applied to the parallel slices, it is not (?) applied to the Field3D data, so interpolating would potentially overwrite the boundary conditions in the parallel slices with junk?

Thinking out loud:

  • When the bc is applied directly to the parallel slices, the boundary condition has to be applied after the parallel slices are calculated.
  • If the bc is applied to the Field3D data (e.g. using to/from field aligned method) it needs to be applied before calculating parallel slices (unless these boundary conditions know to fill in the parallel slices?).
    Supporting both cases neatly seems tricky!

@ZedThree
Copy link
Copy Markdown
Member Author

ZedThree commented Feb 2, 2026

The unfortunately named RGN_NOBNDRY is actually no guards, so it also skips the X (and soon, Z) guards. I'm not sure if this matters in general, except for this specific test, it's more of a matter of duplicating work done on other processors or not. We may or may not want that.

To be precise, ZInterpolation removes the points where i.yp(y_offset) would be in the boundary. This is because in the actual interpolation, we store the i.yp weights at i, which is why we need to remove interior points.

I guess BCs in y should always be applied to the parallel slices? Is the correct order of operations:

  1. apply BCs in X and optionally in Y
  2. calculate parallel slices
  3. apply parallel BCs

I think my confusion here is really just that ShiftedMetric does actually apply in the guards (and boundaries?), but ShiftedMetricInterp doesn't, when you would expect they both should operate over the same regions.

@dschwoerer
Copy link
Copy Markdown
Contributor

Yes, at least for FCI, the mentioned order is the only correct one.
With FCI we cannot (or at least not without huge amount of work) apply y BCs before transform.

One important thing to note is that ZHermiteSpline (the z-only interpolation) explicitly doesn't do anything in any of the guards. This means that f.yup(mesh->yend + 1) does not get filled in.

That feels wrong.

There's a comment the guards should be filled in by the boundary conditions, but I'm not actually sure that's true for the parallel slices.

I am not sure about non-FCI, but does this region get added to the parallel boundary region? If not, it is certainly wrong.
If it does, it still feels wrong, as that means that for localNy=3 and MYG=1, there would nothing ever be done?

@dschwoerer
Copy link
Copy Markdown
Contributor

If I instead comment out the region masking in ZInterpolation, and just use RGN_NOY instead, all the tests seem to pass, including the integrated tests that use MAST grids.

Oh, I missed that.
Yes, just do that, that is what I think should be done anyway. If it just makes everything pass - even better 👍

@johnomotani
Copy link
Copy Markdown
Contributor

I think ShiftedMetricInterp should be doing more or less the same thing as FCI, so would trust what @dschwoerer says. Just to add, this solver was written for consistency with the LaplacePetsc3dAmg/LaplaceHypre3d solvers, but I don't think it's ever been used, so anything that looks like a mistake/bug probably is one!

Copy link
Copy Markdown
Contributor

@dschwoerer dschwoerer left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@bendudson bendudson merged commit 28d3a0d into next Feb 6, 2026
1 check passed
@bendudson bendudson deleted the unit-test-shiftedmetricinterp-debug branch February 6, 2026 17:52
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.

4 participants