View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0001726 | OpenFOAM | Bug | public | 2015-06-01 16:57 | 2015-10-23 08:33 |
| Reporter | jacob | Assigned To | henry | ||
| Priority | normal | Severity | major | Reproducibility | sometimes |
| Status | resolved | Resolution | fixed | ||
| Platform | GNU/Linux | OS | Ubuntu | OS Version | 15.04 |
| Summary | 0001726: Wrong rotationAngle signs in parallel decomposition of cyclicAMI | ||||
| Description | The 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 Reproduce | Depends on decomposition. | ||||
| Additional Information | Moreover, 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. | ||||
| Tags | No tags attached. | ||||
|
|
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? |
|
|
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. |
|
|
area1 does not exist in the current cyclicAMIPolyPatch.C. Please test the latest OpenFOAM-2.3.x |
|
|
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. |
|
|
This looks OK. Could you provide a test-case? |
|
|
|
|
|
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. |
|
|
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) |
|
|
What is the appropriate tolerance for this test? |
|
|
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. |
|
|
Could we avoid the geometric test by doing a reduction on the sizes? |
|
|
Don't see how - if the reduced sizes are 0 there is no problem anyway. |
|
|
|
|
|
I have uploaded another case, that illustrates the problem mentioned in "Additional Information". |
|
|
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? |
|
|
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... |
|
|
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; } |
|
|
@jacob: Does the latest proposal fix the problem for your cases? |
|
|
Yes, the latest proposal fixes both cases. |
|
|
Resolved in OpenFOAM-dev by commit 7595f4c225c525ed95ebea21ec5070fb7216e338 |
| 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 |
|
Note Added: 0004875 | |
| 2015-06-02 14:15 | henry | Note Added: 0004876 | |
| 2015-06-02 15:29 |
|
Note Added: 0004877 | |
| 2015-06-02 15:31 | henry | Note Added: 0004878 | |
| 2015-06-02 16:00 |
|
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 |
|
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 |
