- removed comment
ReflectionSYmmetry does not handle vector groups of vectors correctly
Issue #1236
closed
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)
-
-
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.
-
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.
-
- changed milestone to ET_2013_05
- removed comment
-
- removed comment
- changed watchers to lroberts
-
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.
-
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.
-
- 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.
-
- changed milestone to ET_2013_11
- removed comment
-
- removed comment
Please apply now, and as Erik mentioned, do the same for (Periodic, RotatingSymmetry90, RotatingSymmetry180, etc.)
-
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.
-
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.
-
reporter
- changed status to open
- removed comment
-
reporter
- removed comment
Should also be backported to Gauss.
-
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).
-
- changed status to open
- removed comment
-
reporter
- changed status to resolved
- removed comment
Applied as rev 61 of ReflectionSymmetry.
-
reporter
- edited description
- changed status to closed
- Log in to comment
- Assignee
- –
- Type
- Priority
- Status
- closed
- Component
- Cactus
- Milestone
- ET_2013_11
- Version
- –
- Votes
- 0
- Watchers
- 0
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.