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.

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

Feature: graph representation of the grid #776

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

michal-toth
Copy link

This is my second attempt to create a pull request after the first one got bugged (see #774 ). akva2 doubled it already (#775), so this one is to determine if I can create a working PR. The original description follows:

Graph representation of the grid.
Purpose: Loadbalancing of the grid with wells. Each well is represented with a single vertex in the graph repr. of the grid, therefore no well can be spread over multiple processes.

What is implemented so far:

GraphofGrid -class that stores the representation. Needs a grid (currently CpGrid) to build the graph. Has functions for searching for cells by their global IDs, adding wells, and vertex contraction.
Edge weights (transmissibilities) currently start equal to 1, and are possibly added up when contracted vertices have a common neighbor
wrappers for Zoltan callback functions (not tested with Zoltan yet though)
unit test for the above

@blattms
Copy link
Member

blattms commented Oct 16, 2024

jenkins build this please

@akva2 akva2 mentioned this pull request Oct 16, 2024
@michal-toth michal-toth changed the title Feature/graph of grid second try Feature: graph representation of the grid Oct 16, 2024
@michal-toth michal-toth force-pushed the feature/graph-of-grid-second-try branch from c8be590 to dd7d098 Compare October 17, 2024 12:30
@blattms
Copy link
Member

blattms commented Oct 17, 2024

jenkins build this please

Copy link
Member

@blattms blattms left a comment

Choose a reason for hiding this comment

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

Thanks a lot.
I have not checked the tests very diligently yet I might come back to that later. Just a few comments there for now.

Please note that i might always miss something or misunderstand things.

CMakeLists_files.cmake Outdated Show resolved Hide resolved
opm/grid/GraphOfGrid.cpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.hpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.hpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.hpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGrid.cpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGrid.cpp Outdated Show resolved Hide resolved
tests/test_graphofgrid.cpp Outdated Show resolved Hide resolved
tests/test_graphofgrid.cpp Outdated Show resolved Hide resolved
tests/test_graphofgrid.cpp Show resolved Hide resolved
Contracting an edge joins two vertices into one. Their weight is added up, and the lists of edges is merged. For a common neighbor the edge weights are added up.
…tegration/implementation of wells.

Note: Code debt: Zoltan-specific types are overwritten/hard-coded. Also wrappers have the type of grid hard-coded to CpGrid.
Each well is a set of IDs of its vertices. The graph contains only one ID (the smallest), but functions will now check wells if the sought ID is not in the graph itself.
Wells are assumed disjoint, adding new well checks for intersections and possibly merges wells together. The check for intersections can be switched off to improve performance.
@michal-toth michal-toth force-pushed the feature/graph-of-grid-second-try branch from dd7d098 to 4cabe9a Compare October 21, 2024 15:33
Copy link
Member

@blattms blattms left a comment

Choose a reason for hiding this comment

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

I am sending this now to reduce the feedback delay. I am not finished yet, though. Still need to look at the import / export lists.

opm/grid/GraphOfGrid.hpp Show resolved Hide resolved
opm/grid/GraphOfGrid.hpp Outdated Show resolved Hide resolved
tests/test_graphofgrid.cpp Outdated Show resolved Hide resolved
tests/test_graphofgrid.cpp Show resolved Hide resolved
tests/test_graphofgrid.cpp Outdated Show resolved Hide resolved
tests/test_graphofgrid.cpp Show resolved Hide resolved
@blattms
Copy link
Member

blattms commented Oct 24, 2024

jenkins build this please

@blattms
Copy link
Member

blattms commented Oct 25, 2024

jenkins build this serial please

Copy link
Member

@blattms blattms left a comment

Choose a reason for hiding this comment

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

Nice and I think I am finished.

Last part is mostly minor things and stylistic comments.

opm/grid/GraphOfGridWrappers.hpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.hpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.hpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.hpp Outdated Show resolved Hide resolved
std::sort(addToList.begin(),addToList.end(),[](const auto& a, const auto& b){return std::get<0>(a)<std::get<0>(b);});
int origSize = cellList.size();
int totsize = origSize+addToList.size();
cellList.reserve(totsize);
Copy link
Member

Choose a reason for hiding this comment

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

I think we do know who many entries will be added in the function already at the beginning, right?
In that case we might be able to omit the extra container addToList and push_back to cellList directly.

I might be missing something here, though.

Copy link
Author

Choose a reason for hiding this comment

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

The number of cells to be added to the cellList depends on which wells are in the list. Unless all wells are always in the list we do not know the final size beforehand.

@blattms
Copy link
Member

blattms commented Oct 25, 2024

There are some build failures (due to me, sorry):

/var/lib/jenkins/workspace/opm-grid-PR-builder/tests/test_graphofgrid.cpp:358:38: error: ‘AttributeSet’ has not been declared
  358 |     BOOST_CHECK(std::get<2>(imp[5])==AttributeSet::copy);
      |                                      ^~~~~~~~~~~~

I think you need to explicitly include dune/istl/owneroverlapcopy.hh to fix this.

@michal-toth
Copy link
Author

There are some build failures (due to me, sorry):

/var/lib/jenkins/workspace/opm-grid-PR-builder/tests/test_graphofgrid.cpp:358:38: error: ‘AttributeSet’ has not been declared
  358 |     BOOST_CHECK(std::get<2>(imp[5])==AttributeSet::copy);
      |                                      ^~~~~~~~~~~~

I think you need to explicitly include dune/istl/owneroverlapcopy.hh to fix this.

I will add the include. However, I can not reproduce this error so I can not check if it helped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants