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 of grid: well envelope #811

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

Conversation

michal-toth
Copy link
Contributor

@michal-toth michal-toth commented Dec 4, 2024

This PR adds an interface for using the function addNeighboringCellsToWells of GraphOfGrid that during partitioning merges each well with a layer(s) of cells surrounding it, in order to keep the well further from the subdomain boundary. To use this option, flow must be called with parameter --zoltan-params=*.json taking a json file. The json file then must contain the keyword EnvelopeWellLayers with the number of layers (default 0). The feature is supported only by the partitioner using the GraphOfGrid (--partition-method=3).

In addition, the communication of the well cells (that are not visible to the partitioner) was streamlined, and we added a few more unit tests.

@michal-toth
Copy link
Contributor Author

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.

Looks good. A few comments...

opm/grid/GraphOfGridWrappers.cpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.cpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.cpp Outdated Show resolved Hide resolved
@michal-toth
Copy link
Contributor Author

jenkins build this please

@michal-toth michal-toth changed the title Feature/graph of grid second try Feature/graph of grid: well envelope Dec 9, 2024
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. In general this looks like a very good improvement.

Fist batch of comments. I still need to look at the two test files.

Hopefully there are more useful comments than nitpicking on white space issues. 😅

opm/grid/GraphOfGrid.cpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.cpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.cpp Show resolved Hide resolved
opm/grid/GraphOfGrid.cpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.cpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.cpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.cpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.cpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.hpp Outdated Show resolved Hide resolved
opm/grid/GraphOfGridWrappers.hpp Show resolved Hide resolved
@michal-toth
Copy link
Contributor Author

jenkins build this please

@blattms
Copy link
Member

blattms commented Dec 13, 2024

Thanks for all the changes.
I will insist on the exception, though.
The arguments on performance might be premature optimization. Hence maybe we need to do some realistic benchmarking to persuade us.

@michal-toth
Copy link
Contributor Author

michal-toth commented Dec 16, 2024

After checking the performance of the unordered_map in the extendRootExportList, I looked into map/unordered_map in the GraphOfGrid. There are two instances: the first is the graph itself and stores gID->vertexProperties, while the second is EdgeList inside the vertexProperties and stores gID->EdgeWeight.
Before, we used unordered_map for both, but some testing has shown that using a map for EdgeList (which has usually at most 6 entries) is faster. Using map for the graph itself is worse. Both are about 10% difference in performance. However, using map for both resulted in dramatic performance drop - the compute time doubled. I can not explain why, I tried few more runs after waiting to ensure it is not caused by heat-throttling on my notebook, but got the same results.

@michal-toth
Copy link
Contributor Author

michal-toth commented Dec 16, 2024

Testing the map and unordered_map times:

graph edges Test: Easy Medium Hard 1 Hard 2
un_map un_map 15162 14196684 169561690 185451570
un_map map 13910 12138575 163632397 165122661
map un_map 16852 15992087 191048511 189642649
map map 13460 28522892 331562668 315050777

graph is the whole structure in the GraphOfGrid (except wells), it maps global ID to the vertex properties (that include the vertex weight and the map of neighbors called the edgeList). edges is the edgeList, the map of neighbors mapping (neighbors') global ID to the edge weight (which is e.g. the transmissibility between the vertex and its neighbor).
Time is in microseconds, each run parsed the grid 100 times. Only the creation of the graph was considered, callback functions from Zoltan did not enter the calculation. The reserve for the unordered_map of the graph was added later, so the numbers for the best case can be even a little better.

@michal-toth
Copy link
Contributor Author

jenkins build this please

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