View Issue Details

IDProjectCategoryView StatusLast Update
0001726OpenFOAMBugpublic2015-10-23 08:33
Reporterjacob Assigned Tohenry  
PrioritynormalSeveritymajorReproducibilitysometimes
Status resolvedResolutionfixed 
PlatformGNU/LinuxOSUbuntuOS Version15.04
Summary0001726: Wrong rotationAngle signs in parallel decomposition of cyclicAMI
DescriptionThe cyclicAMI rotationAngle specified by the user is never kept intact: the cyclicAMIPolyPatch::calcTransforms always tries to determine the signs of the angles on its own.

When the mesh is being decomposed for parallel run, it can easily happen that one of the owner/neighbour patches contains no faces. In such cases the heuristic algorithm fails and resulting angles are wrong. This can ultimately lead to a crash of the calculation of addressing.

I think that the user-specified sign should be used in such cases. This is achieved by modifying line 175 of cyclicAMIPolyPatch.C (revision 6586d545)

    if (errorPos < errorNeg)

to

    if (mag(area0) == 0 || mag(area1) == 0 || errorPos <= errorNeg)

Now, if any of the patches is empty (or positive and negative rotations are equally good), the user-specified rotation angle will be used.
Steps To ReproduceDepends on decomposition.
Additional InformationMoreover, when the cyclic AMI patches are curved (e.g. a spiral shape), it can possibly happen that a processor receives such a portion of the patches that it will consider the wrong rotation better than the correct one. Wouldn't it be better to let the user specify the rotationAngle-s already WITH the correct signs for each cyclic AMI patch and drop this buggy algorithm? Only the "revTPos" transform would be used then.

TagsNo tags attached.

Activities

henry

2015-06-01 17:11

manager   ~0004864

In

    if (mag(area0) == 0 || mag(area1) == 0 || errorPos <= errorNeg)

could you clarify what area1 is? It does not appear in the current cyclicAMIPolyPatch.C. Also it is not good to test a scalar for exact equality with 0, it will VERY rarely be the case due to round-off error.

If the issue is associated with patches with 0 faces wouldn't it be better use this condition to select alternative behavior?

jacob

2015-06-01 18:57

reporter   ~0004865

The variable "area0" is the surface area of the owner patch (defined in cyclicAMIPolyPatch.C), "area1" is the surface area of the neighbour patch that would be defined similarly (gSum(half1Areas)).

But you are right: A simple check of half0Areas.size() and half1Areas.size() would be more reliable.

henry

2015-06-01 18:59

manager   ~0004866

area1 does not exist in the current cyclicAMIPolyPatch.C. Please test the latest OpenFOAM-2.3.x

jacob

2015-06-01 19:32

reporter   ~0004867

True, it doesn't exist, it was my proposition. But let's forget it, your solution is better: Let's use the condition in the form

    if (half0Areas.size() == 0 || half1Areas.size() == 0 || errorPos <= errorNeg)

This works for me with latest 2.3.x-6586d545, too.

henry

2015-06-01 22:18

manager   ~0004869

This looks OK. Could you provide a test-case?

jacob

2015-06-02 13:42

reporter  

AMI-test-0.tar.gz (6,155 bytes)

jacob

2015-06-02 13:47

reporter   ~0004874

I have uploaded a small test case where the parallel decomposition is done manually (i.e. using decompositionData) to achieve a situation where some of the processor portions of the AMI patches keep their original rotationAngle while the others flip sign, leading to inconsistent decomposition and crash.

The test case fails on 2.3.x-6586d545 (with error "Unable to set source and target faces"), but works well when the above proposed change is made.

user4

2015-06-02 14:05

  ~0004875

Using local information (half0Areas.size()) might lead to inconsistent decisions being made since as you mentioned some processors would have zero size (and not flip) whereas other might decide to flip.

The first fix seems a better one but would depend on tolerances (which are already being used, e.g. line 186: mag(area0) + ROOTVSMALL)

henry

2015-06-02 14:15

manager   ~0004876

What is the appropriate tolerance for this test?

user4

2015-06-02 15:29

  ~0004877

I'd say if we want to support linear dimensions > SMALL then the check on the areas should be sqr(SMALL) (=1e-30). It should only be large enough such that we're not deciding based on numerical noise.

The second question is whether to have the automatic flipping silent (as is now), output a warning or throw an error. Since this case not very common (? I haven't checked the case though) I vote to keep it silent or output a warning.

henry

2015-06-02 15:31

manager   ~0004878

Could we avoid the geometric test by doing a reduction on the sizes?

user4

2015-06-02 16:00

  ~0004879

Don't see how - if the reduced sizes are 0 there is no problem anyway.

jacob

2015-06-03 09:04

reporter  

AMI-test-1.tar.gz (5,530 bytes)

jacob

2015-06-03 09:05

reporter   ~0004884

Last edited: 2015-06-03 09:37

I have uploaded another case, that illustrates the problem mentioned in "Additional Information".

henry

2015-06-04 14:34

manager   ~0004886

I am testing with the test Mattijs suggests:

if (mag(area0) < SMALL*SMALL || errorPos < errorNeg)

and with this AMI-test-0 runs but AMI-test-1 does not. Are you able to run AMI-test-1 with any of the options proposed so far?

jacob

2015-06-04 14:46

reporter   ~0004888

Last edited: 2015-06-04 14:56

There is a diff file in both case directories that makes them run.

In the first example (AMI-test-0) it is enough to take care of the null patches using any fix that was mentioned.

In the second example (AMI-test-1) it was necessary to comment out the whole algorithm that modifies signs of the angles to force cyclicAMI to use the user-given signs. I don't know of any simple way how to correct the code in this case.

I might have reported these bugs separately, but both are connected to the same algorithm that (incorrectly) sets sign of rotationAngle...

user4

2015-06-05 21:18

  ~0004890

One of the errors should be (almost) zero in 'normal' operation so we only flip the sign if the neg error is less than the positive AND close to zero. Below will still output the warning though in decomposePar - it just disables the flipping.

Something like

                scalar scaledErrorPos = errorPos/(mag(area0) + ROOTVSMALL);
                scalar scaledErrorNeg = errorNeg/(mag(area0) + ROOTVSMALL);

                // One of the errors should be (close to) zero. If this is
                // the reverse transformation flip the rotation angle.
                revT = revTPos;
                if (errorPos > errorNeg && scaledErrorNeg <= matchTolerance())
                {
                    revT = revTNeg;
                    rotationAngle_ *= -1;
                }

henry

2015-10-22 10:20

manager   ~0005467

@jacob: Does the latest proposal fix the problem for your cases?

jacob

2015-10-22 14:59

reporter   ~0005480

Yes, the latest proposal fixes both cases.

henry

2015-10-23 08:33

manager   ~0005482

Resolved in OpenFOAM-dev by commit 7595f4c225c525ed95ebea21ec5070fb7216e338

Issue History

Date Modified Username Field Change
2015-06-01 16:57 jacob New Issue
2015-06-01 17:11 henry Note Added: 0004864
2015-06-01 18:57 jacob Note Added: 0004865
2015-06-01 18:59 henry Note Added: 0004866
2015-06-01 19:32 jacob Note Added: 0004867
2015-06-01 22:18 henry Note Added: 0004869
2015-06-02 13:42 jacob File Added: AMI-test-0.tar.gz
2015-06-02 13:47 jacob Note Added: 0004874
2015-06-02 14:05 user4 Note Added: 0004875
2015-06-02 14:15 henry Note Added: 0004876
2015-06-02 15:29 user4 Note Added: 0004877
2015-06-02 15:31 henry Note Added: 0004878
2015-06-02 16:00 user4 Note Added: 0004879
2015-06-03 09:04 jacob File Added: AMI-test-1.tar.gz
2015-06-03 09:05 jacob Note Added: 0004884
2015-06-03 09:37 jacob Note Edited: 0004884
2015-06-04 14:34 henry Note Added: 0004886
2015-06-04 14:46 jacob Note Added: 0004888
2015-06-04 14:47 jacob Note Edited: 0004888
2015-06-04 14:56 jacob Note Edited: 0004888
2015-06-05 21:18 user4 Note Added: 0004890
2015-10-22 10:20 henry Note Added: 0005467
2015-10-22 14:59 jacob Note Added: 0005480
2015-10-23 08:33 henry Note Added: 0005482
2015-10-23 08:33 henry Status new => resolved
2015-10-23 08:33 henry Resolution open => fixed
2015-10-23 08:33 henry Assigned To => henry