View Issue Details

IDProjectCategoryView StatusLast Update
0002226OpenFOAMBugpublic2016-09-29 14:01
ReporterMattijsJ Assigned Tohenry  
PrioritylowSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
PlatformGNU/LinuxOSOpenSuSEOS Version13.2
Product Versiondev 
Fixed in Versiondev 
Summary0002226: GAMGagglomeration does not stop when nCellsInCoarsestLevel 1
Descriptionset nCellsInCoarsestLevel to 1. The pairGAMGagglomeration keeps on iterating until it reaches the maxLevel (50). Hence it creates lots of unnecessary levels.

Attached a dump of the levels (with 'processorAgglomerator none')

The easiest fix is probably to adapt the logic in GAMGAgglomeration::continueAgglomerating to include the old number of cells.
TagsNo tags attached.

Activities

MattijsJ

2016-09-01 14:47

reporter  

henry

2016-09-03 16:06

manager   ~0006805

Can you provide a patch for this?

It is unlikely that setting nCellsInCoarsestLevel to 1 is a good idea anyway.

MattijsJ

2016-09-03 20:50

reporter  

GAMGAgglomeration.patch (2,752 bytes)   
diff --git a/src/OpenFOAM/matrices/lduMatrix/solvers/GAMG/GAMGAgglomerations/GAMGAgglomeration/GAMGAgglomeration.C b/src/OpenFOAM/matrices/lduMatrix/solvers/GAMG/GAMGAgglomerations/GAMGAgglomeration/GAMGAgglomeration.C
index 6f7665b..517aad3 100644
--- a/src/OpenFOAM/matrices/lduMatrix/solvers/GAMG/GAMGAgglomerations/GAMGAgglomeration/GAMGAgglomeration.C
+++ b/src/OpenFOAM/matrices/lduMatrix/solvers/GAMG/GAMGAgglomerations/GAMGAgglomeration/GAMGAgglomeration.C
@@ -206,11 +206,14 @@ void Foam::GAMGAgglomeration::compactLevels(const label nCreatedLevels)
 
 bool Foam::GAMGAgglomeration::continueAgglomerating
 (
+    const label nFineCells,
     const label nCoarseCells
 ) const
 {
     // Check the need for further agglomeration on all processors
-    bool contAgg = nCoarseCells >= nCellsInCoarsestLevel_;
+    bool contAgg =
+        (nCoarseCells >= nCellsInCoarsestLevel_)
+     && (nCoarseCells < nFineCells);
     mesh().reduce(contAgg, andOp<bool>());
     return contAgg;
 }
diff --git a/src/OpenFOAM/matrices/lduMatrix/solvers/GAMG/GAMGAgglomerations/GAMGAgglomeration/GAMGAgglomeration.H b/src/OpenFOAM/matrices/lduMatrix/solvers/GAMG/GAMGAgglomerations/GAMGAgglomeration/GAMGAgglomeration.H
index fd94b55..7019c67 100644
--- a/src/OpenFOAM/matrices/lduMatrix/solvers/GAMG/GAMGAgglomerations/GAMGAgglomeration/GAMGAgglomeration.H
+++ b/src/OpenFOAM/matrices/lduMatrix/solvers/GAMG/GAMGAgglomerations/GAMGAgglomeration/GAMGAgglomeration.H
@@ -152,7 +152,11 @@ protected:
         void compactLevels(const label nCreatedLevels);
 
         //- Check the need for further agglomeration
-        bool continueAgglomerating(const label nCoarseCells) const;
+        bool continueAgglomerating
+        (
+            const label nCells,
+            const label nCoarseCells
+        ) const;
 
         //- Gather value from all procIDs onto procIDs[0]
         template<class Type>
diff --git a/src/OpenFOAM/matrices/lduMatrix/solvers/GAMG/GAMGAgglomerations/pairGAMGAgglomeration/pairGAMGAgglomerate.C b/src/OpenFOAM/matrices/lduMatrix/solvers/GAMG/GAMGAgglomerations/pairGAMGAgglomeration/pairGAMGAgglomerate.C
index bf4c695..42133e6 100644
--- a/src/OpenFOAM/matrices/lduMatrix/solvers/GAMG/GAMGAgglomerations/pairGAMGAgglomeration/pairGAMGAgglomerate.C
+++ b/src/OpenFOAM/matrices/lduMatrix/solvers/GAMG/GAMGAgglomerations/pairGAMGAgglomeration/pairGAMGAgglomerate.C
@@ -54,7 +54,7 @@ void Foam::pairGAMGAgglomeration::agglomerate
             *faceWeightsPtr
         );
 
-        if (continueAgglomerating(nCoarseCells))
+        if (continueAgglomerating(finalAgglomPtr().size(), nCoarseCells))
         {
             nCells_[nCreatedLevels] = nCoarseCells;
             restrictAddressing_.set(nCreatedLevels, finalAgglomPtr);
GAMGAgglomeration.patch (2,752 bytes)   

MattijsJ

2016-09-03 20:57

reporter   ~0006811

On very large clusters (6k procs) we've seen a benefit - smaller nCellsInCoarsestLevel the better.

I've attached a patch but not happy with the logic inside continueAgglomerating.

- with the old logic we stop if one processor < nCellsInCoarsestLevel
- do we want all processors instead?
- the additional logic should be to continue if at least one processor has nCoarseCells<nFineCells.
- an alternative is to work on averages:
    label nTotalCoarseCells = returnReduce(nCoarseCells, sumOp<label>());
    if (nTotalCoarseCells >= Pstream::nProcs()*nCellsInCoarsestLevel_)
    {
        return true;
    }
    label nTotalFineCells = returnReduce(nFineCells, sumOp<label>());
    return ((nTotalCoarseCells < nTotalFineCells);

henry

2016-09-04 21:10

manager   ~0006821

Which option provide best performance on a range of cases, decompositions and number of processors?

MattijsJ

2016-09-06 12:21

reporter   ~0006831

Not sure.

1. for the nCellsInCoarsestLevel_ there are three options:
1A as is now, if one processor is below nCellsInCoarsestLevel_ stop
1B if all processors are below nCellsInCoarsestLevel_ stop
1C if average is below nCellsInCoarsestLevel_ stop

for 'normal' decompositions they all behave the same but if we weight the decomposition (e.g. by number of lagrangian particles) and one processor has 100 cells and all others 1M it is the 100 cells processor that stops all agglomeration (with 1A).

2. Automatic stop (nCoarseCells=nFineCells): this should be over all processors so only stop if none of the processors has changed. So either

2A: returnReduce(nCoarseCells < nFineCells, orOp<bool>())

or

2B: returnReduce(nCoarseCells, sumOp<label>()) < returnReduce(nFineCells, sumOp<label>())

2B will also work once we go to agglomeration methods where cells can migrate across processors.

My vote is for the 1B + 2B.

henry

2016-09-06 12:33

manager   ~0006832

I agree with this proposal. Which option is implemented in the patch you supplied?

MattijsJ

2016-09-06 12:51

reporter   ~0006834

Not that one. Just apply the patch and replace the logic in continueAgglomerating with

label nTotalCoarseCells = returnReduce(nCoarseCells, sumOp<label>());
if (nTotalCoarseCells >= Pstream::nProcs()*nCellsInCoarsestLevel_)
{
    return true;
}
label nTotalFineCells = returnReduce(nFineCells, sumOp<label>());
return nTotalCoarseCells < nTotalFineCells;


You could cache the nTotalCoarseCells in the calling function and use it to avoid the reduction for nTotalFineCells but I don't think that is worth the effort.

henry

2016-09-06 13:31

manager   ~0006836

Resolved by commit faa6d0e4dad9530a05fc4015d3af213950e7df52

MattijsJ

2016-09-16 10:11

reporter   ~0006869

Logic is slightly wrong and should only test for no-change if nCoarseCells is still larger than wanted nCellsInCoarsestLevel. Below works:

    const label nTotalCoarseCells = returnReduce(nCoarseCells, sumOp<label>());
    if (nTotalCoarseCells < Pstream::nProcs()*nCellsInCoarsestLevel_)
    {
        return false;
    }
    else
    {
        const label nTotalFineCells = returnReduce(nFineCells, sumOp<label>());
        return nTotalCoarseCells < nTotalFineCells;
    }

MattijsJ

2016-09-29 13:59

reporter   ~0006942

Can be closed. Solved by b4bf4be70081219fe654ba092e766b718beabd7c.

Issue History

Date Modified Username Field Change
2016-09-01 14:47 MattijsJ New Issue
2016-09-01 14:47 MattijsJ File Added: log.potentialFoam_nCells1
2016-09-03 16:06 henry Note Added: 0006805
2016-09-03 20:50 MattijsJ File Added: GAMGAgglomeration.patch
2016-09-03 20:57 MattijsJ Note Added: 0006811
2016-09-04 21:10 henry Note Added: 0006821
2016-09-06 12:21 MattijsJ Note Added: 0006831
2016-09-06 12:33 henry Note Added: 0006832
2016-09-06 12:51 MattijsJ Note Added: 0006834
2016-09-06 13:31 henry Assigned To => henry
2016-09-06 13:31 henry Status new => resolved
2016-09-06 13:31 henry Resolution open => fixed
2016-09-06 13:31 henry Fixed in Version => dev
2016-09-06 13:31 henry Note Added: 0006836
2016-09-16 10:11 MattijsJ Status resolved => feedback
2016-09-16 10:11 MattijsJ Resolution fixed => reopened
2016-09-16 10:11 MattijsJ Note Added: 0006869
2016-09-29 13:59 MattijsJ Note Added: 0006942
2016-09-29 13:59 MattijsJ Status feedback => assigned
2016-09-29 14:01 henry Status assigned => resolved
2016-09-29 14:01 henry Resolution reopened => fixed