ReflectionSYmmetry does not handle vector groups of vectors correctly

Create issue
Issue #1236 closed
Roland Haas created an issue

Currently we use two different way to define vectors in Cactus:

CCTK_REAL vel[3] "some vector variable"

and

CCTK_REAL vel
{
  velx, vely, velz
} "some other vector variable"

both of which work with ReflectionSymmetry since it only looks at the ordering of variables in a group (ie does vi=CCTK_FirstVarInGroup() then assumes vi is the x component vi+1 the y component and vi+2 the z component).

For a group of related vectors we'd like to use

CCTK_REAL nvel[42]
{
  velx, vely, velz
} "bunch of vector"

which almost works in the indeed CCTK_VarIndex("velx[0]") + 1 == CCTK_VarIndex("vely[0]"). Unfortunately the symmetry thorns assume that vector groups contain precisely three members. A simple fix seems to instead assert() that the number of members is divisible by 3 and then loop over them in chunks of three.

Keyword: RelfectionSymmetry
Keyword: RotatingSymmetry180
Keyword: RotatingSymmetry90

Comments (18)

  1. Erik Schnetter
    • removed comment

    Instead of checking for divisible by three, please check whether the group is a vector group and what the vector length is (see cctk_Group.h, in particular the cGroup data structure).

    Just looping is not a good idea, since boundary conditions can be requested per variable. In this case, symmetries should be applied only once to each variable. Instead of looping, we could use the vector length as distance between the variables. For example, the parameter file may request boundary conditions just for some variables in a group.

    I assume that, in all cases, we would be allowing cases that were disallowed before. Thus, backward compatibility should not be an issue.

  2. Roland Haas reporter
    • removed comment

    Ok. I will check that the number of group members divided by the vector length is the correct number of tensor components. Looping seems to be not required. ReflectionSymmetry contains a routine to apply the boundary condition to a single varindex and that one can be adapted simply by taking care to compute the proper tensor component (via a bunch of "% numcomps"). I'll propose a patch later.

  3. Roland Haas reporter
    • removed comment

    I attach my current attempt at implementing this. It does not break current tests but I have not tested it with an array of vectors yet (lacking a test case right now). If someone else wants to give it a go, please feel free to do so, otherwise I will get back to it once this is first actually needed in the simulations.

  4. Roland Haas reporter
    • removed comment

    Adding Luke Roberts to cc'ed list since he is the only person currently using this. Once this works for him, I'll try and get the patch into the svn repository. Tests might still be an issue since the user code right now is experimental and probably requires a number of daily changing other thorns.

  5. Roland Haas reporter
    • changed status to open
    • removed comment

    Luke tells me that the new code does seem to work for him. No tests that could be used in the ET yet (since his code is all new and changes daily).

    Ok, to apply (it passes all the ET tests, we are "only" lacking tests for the new feature)?

    I will provide a test for the new features once they are used by code in ET/svn.

  6. Erik Schnetter
    • changed status to open
    • removed comment

    Since this is to be used with experimental code, I suggest applying it after the release.

    The other symmetry thorns (Periodic, RotatingSymmetry90, RotatingSymmetry180, etc.) should receive the same correction.

    Okay to apply after the release.

  7. Frank Löffler
    • removed comment

    Please apply now, and as Erik mentioned, do the same for (Periodic, RotatingSymmetry90, RotatingSymmetry180, etc.)

  8. Roland Haas reporter
    • changed status to resolved
    • removed comment

    Applied as rev 58. I created a new ticket for Periodic, RotatingSymmetry90, RotatingSymmetry180, etc. as a reminder for myself to work on this.

  9. Roland Haas reporter
    • changed status to open
    • removed comment

    Apparently I was not being all too careful when testing the statement that CCTK_VarIndex("velx[0]") + 1 == CCTK_VarIndex("vely[0]") for a group of vectors. This is actually incorrect and the patch that was committed thus is also incorrect (though simple vector groups vel[i] or (betax,betay,betaz) are not affected).

    The attached patch uses the correct algorithm, which uses that the variables are not laid out as

    fnux[0], fnuy[0], fnuz[0], fnux[1], ...
    

    but instead as

    fnux[0], fnux[1], fnux[2], fnuy[0], ...
    

    The essentially replaces "vi % numcomps" by "vi / vectorlength" with special case for vel[i] type groups that are vectors of scalars.

  10. Roland Haas reporter
    • removed comment

    Replying to [comment:14 rhaas]:

    Should also be backported to Gauss. scratch this. the original patch did not make it into Gauss (luckily).

  11. Log in to comment