Synchronize cell ids #854
Synchronize cell ids #854
Conversation
aritorto
force-pushed
the
communicateIds
branch
5 times, most recently
from
d25d839 to
bef7f75
Compare
|
jenkins build this serial please |
1 similar comment
|
jenkins build this serial please |
There was a problem hiding this comment.
Thanks. This is good first version but we probably will need to refactor it to a version without any broadcasts of large containers later.
We still need to make some changes before merging.
We should concentrate on one direction (data_ -> distributed_data_) for now and make that clear from the function names and not call currentData() in the added code.
opm/grid/CpGrid.hpp
Outdated
| @@ -931,6 +931,25 @@ namespace Dune | |||
| const int& preAdaptMaxLevel, | |||
| const int& newLevels); | |||
|
|
|||
| void lgrCommunication(const std::vector<int>& assignRefinedLevel, | |||
There was a problem hiding this comment.
Please use a more speaking function name.
opm/grid/CpGrid.hpp
Outdated
| /// LGRs (Local Grid Refinements) can be added either in the undistributed view first and then in the distributed view, | ||
| /// or vice versa. This method ensures consistency by rewriting the global cell ids in the distributed view | ||
| /// using the corresponding ids from the undistributed view. | ||
| void syncCellIds(); |
| void CpGrid::getFirstChildGlobalIds([[maybe_unused]] std::vector<int>& parentToFirstChildGlobalIds) | ||
| { | ||
| #if HAVE_MPI | ||
| switchToGlobalView(); |
There was a problem hiding this comment.
This should probably not be called as it changes other parts of the grid. It changes the result of currentData()!
Please find another way.
There was a problem hiding this comment.
I actually use the fact that changes currentData() , to make sure in the lines below that currentDAta points at data
|
|
||
| auto size = comm().max(parentToFirstChildGlobalIds.size()); | ||
|
|
||
| switchToDistributedView(); |
There was a problem hiding this comment.
Same as above.
Basically, we rely on calling different methods in a certain order or things break.
aritorto
force-pushed
the
communicateIds
branch
from
f9a4b77 to
27ddac5
Compare
Except from this point, all comments have been addressed. In my understanding, switching views gives the code more flexibility in the sense that synchronization of cell ids (from data_ to distributed_data_) is possible for both situations: (A) 1. add LGRs in global grid, 2. switch to distributed view, 3. add LGRs in distributed view, or (B) 1. distribute grid, 2. add LGRs in distributed grid, 3. switch to global view, 4. add LGRs in global view. Maybe there is a way of refactoring the added code that keeps this flexibility and remove the use of switching views. Thanks for the review, @blattms! |
|
jenkins build this serial please |
| faceIds, | ||
| vertexIds); | ||
|
|
||
| populateCellIndexSetRefinedGrid(level); |
There was a problem hiding this comment.
Sorry, I missed this one during the first review. It is a bit unclear to me how this behaves:
- The method does not clear the interface before adding. I am not sure what happens with our approach, might be one of these
- We have multiple entries with the same global id
- We only have one, but cannot be sure which one of the two wins.
- RemoteIndices using the ParallelIndexSet will be broken. Hence we will need to rebuild/update them. Not sure whether there are some and whether we use those later.
I guess the easy fix is to clear the index set in the function before adding and then maybe rebuild remote indices.
In the long run this should have its own way of doing things as we actually need to do less then in populateCellIndexSetRefinedGrid.
Synchronizes cell global ids across processes after load balancing and refinement.
LGRs (Local Grid Refinements) can be added either in the undistributed view first and then in the distributed view, or vice versa. The method CpGrid::syncCellIds() ensures consistency by rewriting the global cell ids in the distributed view using the corresponding ids from the undistributed view.
Note: vertex ids are nor rewritten in this PR.
Not relevant for the Reference Manual.