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

Twistshift into paralleltransforms #1759

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

Conversation

johnomotani
Copy link
Contributor

Proposed resolution to #375.

ParallelTransform classes seem like the logical place for twist-shift logic. This PR creates a ParallelTransform::applyTwistShift method, which is called during communications by BoutMesh::wait through Field3D::applyTwistShift.

  • ParallelTransformIdentity applies twist-shift to all fields (if TwistShift==true).
  • ShiftedMetric applies twist-shift only to field-aligned fields (checking TwistShift==true if a field-aligned field is found).
  • FCITransform does not twist-shift any field, checking that no field passed to it is field-aligned.
    The points where twist-shifts should be applied are given by TwistShiftDown and TwistShiftUp regions created by BoutMesh.

Fixes a bug where twist-shift would not be applied when using ParallelTransformIdentity for fields with YDirectionType::Standard - it should be applied then because all fields are field-aligned when using ParallelTransformIdentity.

Also moves the code for shiftZ functions from field3d.hxx/field3d.cxx into methods ParallelTransform, which are disabled for FCITransform (where presumably shifting in Z with FFTs doesn't make sense). Free-function shiftZs are still present, but deprecated.

This PR increases rather than decreases the number of lines... so does it seem more logical and simpler? If not I can make a fix for the bug mentioned above as a different PR.

Closes #375.

Twist-shift at branch cuts not applied by a method
ParallelTransform::applyTwistShift, which is called during
communications for Field3Ds in BoutMesh::wait() (via
Field3D::applyTwistShift() method):
- ParallelTransformIdentity applies twist-shift to all fields (if
  TwistShift==true).
- ShiftedMetric applies twist-shift only to field-aligned fields
  (checking TwistShift==true if a field-aligned field is found).
- FCITransform does not twist-shift any field, checking that no field
  passed to it is field-aligned.

The guard cells where a twist shift is needed are specified by
'Region<Ind2D>'s "TwistShiftDown" and "TwistShiftUp" created by
BoutMesh.
@johnomotani johnomotani added the proposal A code/feature outline proposal label Jun 30, 2019
@ZedThree
Copy link
Member

ZedThree commented Jul 3, 2019

Thanks @johnomotani ! This is a nice proof-of-concept, but I'd quite like to keep the identity transform as close to a no-op as possible. What if we instead add two more transforms: TwistShift and ShiftedWithTwistShift? The latter could inherit from ShiftedMetric. For these two transforms, calcParallelSlices would apply twist-shift. This would then avoid the need for both the FCI transform and the fields to know anything about twist-shift, while keeping the logic "at home" in the transforms.

@d7919 has also suggested moving the twist-shift stuff into the boundaries. Currently we don't add boundaries for the periodic Y parts, but we could do so, and then have twist-shift as a boundary condition. This also feels like a natural home for twist-shift, as when it is required, the mesh is not strictly periodic.

@johnomotani
Copy link
Contributor Author

@ZedThree those both sound like good ideas.

In the first case, I think having the twist shift in the basic ShiftedMetric class makes sense, rather than having an extra ShiftedWithTwistShift, as ShiftedMetric already has to deal with both fields in field-aligned and orthogonal-x-z coordinates, and the twist-shift is just part of handling field-aligned fields. A TwistShift class separate from the identity makes sense though.

Logically it would make sense to have the twist-shift be a boundary condition. On the other hand, in cases where twist-shift is needed, it is the only correct thing to do, so we don't need to allow the user to choose between different y-boundary conditions for the closed field line region; my preference would be to not provide options that could be set wrong when there is only one correct thing to do.

So at the moment my vote would go to the option of adding a TwistShift parallel transform. @ZedThree @d7919 @bendudson what do you think?

@dschwoerer dschwoerer added the merge-conflicts PR with significant merge conflicts that need to be resolved label Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-conflicts PR with significant merge conflicts that need to be resolved proposal A code/feature outline proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants