Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes for more resonable partitioning: #813

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Changes for more resonable partitioning: #813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions opm/grid/CpGrid.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1216,10 +1216,10 @@ namespace Dune
/// \param overlapLayers The number of layers of cells of the overlap region (default: 1).
/// \param partitionMethod The method used to partition the grid, one of Dune::PartitionMethod
/// \warning May only be called once.
bool loadBalance(int overlapLayers=1, int partitionMethod = Dune::PartitionMethod::zoltan, double imbalanceTol = 1.1)
bool loadBalance(int overlapLayers=1, int partitionMethod = Dune::PartitionMethod::zoltan, double imbalanceTol = 1.1, bool addCorners = true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is an opportunity to get rid of the default arguments?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. But probably that can be done separately.

{
using std::get;
return get<0>(scatterGrid(defaultTransEdgeWgt, false, nullptr, {}, false, nullptr, true, overlapLayers, partitionMethod, imbalanceTol));
return get<0>(scatterGrid(defaultTransEdgeWgt, false, nullptr, {}, false, nullptr, addCorners, overlapLayers, partitionMethod, imbalanceTol));
}

// loadbalance is not part of the grid interface therefore we skip it.
Expand Down
3 changes: 2 additions & 1 deletion opm/grid/common/GridPartitioning.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,8 @@ void addOverlapLayer(const CpGrid& grid, int index, const CpGrid::Codim<0>::Enti
{
// Note: multiple adds for same process are possible
exportList.emplace_back(nb_index, owner, AttributeSet::copy);
exportList.emplace_back(index, cell_part[nb_index], AttributeSet::copy);
// not need or check if
//exportList.emplace_back(index, cell_part[nb_index], AttributeSet::copy);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not needed? Should it not be symmetrical?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand this fully, but this will be done for all processes so this will be added when the other partition is processed anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If one already have added overlap one will add a copy to an other partitions potential on a cell which already is owner, which make an error. The Overlap code is probably has bed used and tested to little. In some tests parallel indexsets seems to have a problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which make an error.

What do you mean by this?
If I recall correctly: If a cell was added as owner than all additional occurrences as copy will be neglected.

If I am wrong then please add a failing test case demonstrating this first.

The idea of the index sets used is that if we have an index then we now where that index is present in addition. We might be breaking this guarantee here...

if ( recursion_deps>0 )
{
// Add another layer
Expand Down
2 changes: 1 addition & 1 deletion opm/grid/common/ZoltanPartition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ void setDefaultZoltanParameters(Zoltan_Struct* zz) {
Zoltan_Set_Param(zz, "CHECK_GRAPH", "2");
Zoltan_Set_Param(zz,"EDGE_WEIGHT_DIM","0");
Zoltan_Set_Param(zz, "OBJ_WEIGHT_DIM", "0");
Zoltan_Set_Param(zz, "PHG_EDGE_SIZE_THRESHOLD", ".35"); /* 0-remove all, 1-remove none */
Zoltan_Set_Param(zz, "PHG_EDGE_SIZE_THRESHOLD", "1"); /* 0-remove all, 1-remove none */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the observations that favor this change? Have you observed problems with too many of the graph edges being cut? I must admit I do not know how to interpret the threshold: will 0.35 cut 35% of the edges?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the effect of setting this to 1 is that no (hyper-)edges will be omitted from the graph, even if the number of vertices on the edge is a large amount of the total amount of vertices in the graph (used to be 35%). I believe this is intended to detect all-to-all-ish connections that would be a problem for the partitioning algorithm, since all partitions would be penalized for this cut.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem on small cases or "dead ends" i.e 1D is that every second cell is on different partitions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we really optimizing for those cases? Are there performance penalties for other more relevant cases due to this?

}


Expand Down
4 changes: 2 additions & 2 deletions opm/grid/cpgrid/CpGrid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ CpGrid::scatterGrid(EdgeWeightMethod method,
// Silence any unused argument warnings that could occur with various configurations.
static_cast<void>(wells);
static_cast<void>(transmissibilities);
static_cast<void>(overlapLayers);
//static_cast<void>(overlapLayers);
static_cast<void>(method);
static_cast<void>(imbalanceTol);

Expand Down Expand Up @@ -373,7 +373,7 @@ CpGrid::scatterGrid(EdgeWeightMethod method,

// first create the overlap
auto noImportedOwner = addOverlapLayer(*this, computedCellPart, exportList, importList, cc, addCornerCells,
transmissibilities);
transmissibilities, overlapLayers);
// importList contains all the indices that will be here.
auto compareImport = [](const std::tuple<int,int,char,int>& t1,
const std::tuple<int,int,char,int>&t2)
Expand Down