Fixes issues for mult pv #916
Conversation
-- still observer some race condition on strange files with temperature
hnil
added
the
manual:irrelevant
There was a problem hiding this comment.
Thanks a lot. There is certainly a bugfix in here.
- Please elaborate a bit more about the fix in the description (What was the issue with MULTPV?)
- please extend test/test_minpvprocessor.cpp with tests covering the behvior (and failing with out)
- we might need a few small regression tests to use to check this changes against other simulators. Please reach out if somebody else should do that. We do have good people for that
- please remove unrelated changes in files other than MinpvProcessor
| bool c_thin = (thickness[c] <= z_tolerance); | ||
| bool c_thin_inactive = !c_active && c_thin; | ||
| bool c_low_pv_active = pv[c] < minpvv[c] && c_active; | ||
| bool c_low_pv_active = (pv[c] < minpvv[c] && c_active) || (c_thin && c_active); |
There was a problem hiding this comment.
This (and everything below) breaks the variable names reflecting the content. Should be fixed. Holds also for most of the occurrences below.
So basically the if below checks whether the pore volume is below the threshold or the cell is thin (no matter if it is active or inactive).
We should make this clear from the variable name and if statements.
There was a problem hiding this comment.
I am not sure about the added condition. Are you sure that this is as it should be? Why?
I guess we need some regression test cases with active cells to check whether they vanish and NNCs are created.
|
|
||
| // create nnc if false or merge the cells if true | ||
| if (mergeMinPVCells && c_low_pv_active) { | ||
| if (mergeMinPVCells){// && c_low_pv_active) { |
There was a problem hiding this comment.
This looks a bit suspicious. Normally this is false by default, right?
| // however zcorn inbetween is not modified to make zcorn sorted | ||
| // Set lower k coordinates of cell below to upper cells's coordinates. | ||
| // i.e fill the void using the cell below | ||
| if (kk==0 || kk_iter == dims_[2]) { |
There was a problem hiding this comment.
| if (kk==0 || kk_iter == dims_[2]) { | |
| // no top or bottom cell, nothing to do. | |
| if (kk==0 || kk_iter == dims_[2]) { |
| above_active = actnum.empty() || actnum[c_above]; | ||
| above_inactive = !actnum.empty() && !actnum[c_above]; | ||
| auto above_significant_pv = pv[c_above] > minpvv[c_above]; | ||
| auto above_significant_pv = !((pv[c_above] < minpvv[c_above]) || (thickness[c_above] < z_tolerance)); |
There was a problem hiding this comment.
Why use the variable defined above?
| auto above_significant_pv = !((pv[c_above] < minpvv[c_above]) || (thickness[c_above] < z_tolerance)); | |
| auto above_significant_pv = !above_small_pv; |
| else | ||
| { | ||
| if (kk < dims_[2] - 1 && (actnum.empty() || actnum[c]) && pv[c] > minpvv[c] && | ||
| if (kk < dims_[2] - 1 && (actnum.empty() || actnum[c]) && !((pv[c] < minpvv[c]) || (thickness[c] < z_tolerance)) && |
There was a problem hiding this comment.
I think writing this without negating is easier to read.
| if ((actnum.empty() || actnum[c_below]) && pv[c_below] > minpvv[c_below]) | ||
| if ( (actnum.empty() || actnum[c_below]) | ||
| && | ||
| !((pv[c_below] < minpvv[c_below]) || (thickness[c_below] < z_tolerance) ) ) |
| //bool | ||
| above_small_pv = (pv[c_above] < minpvv[c_above]) || (thickness[c_above] < z_tolerance); | ||
| bool below_small_pv = (pv[c_below] < minpvv[c_below]) || (thickness[c_below] < z_tolerance); | ||
| if ( nnc_allowed && | ||
| (actnum.empty() || (actnum[c_above] && actnum[c_below])) && | ||
| pv[c_above] > minpvv[c_above] && pv[c_below] > minpvv[c_below]) { | ||
| !(above_small_pv) && !(below_small_pv) ){ |
There was a problem hiding this comment.
It feels hard to follow all these negations.
//bool
auto above_big_pv = (pv[c_above] >= minpvv[c_above]) && (thickness[c_above] >= z_tolerance);
bool below_big_pv = (pv[c_below] >= minpvv[c_below]) && (thickness[c_below] >= z_tolerance);
if ( nnc_allowed &&
(actnum.empty() || (actnum[c_above] && actnum[c_below])) &&
above_big_pv &&! below_bigl_pv ){
| auto owner = cell_part[index]; | ||
| exportProcs.insert(std::make_pair(owner, 0)); | ||
| if ( trans ) { | ||
| if ( trans && false) { |
There was a problem hiding this comment.
This seems unrelated and might be better in another PR.
| return transMult.getMultiplier(cartindex, ::Opm::FaceDir::ZPlus) * | ||
| transMult.getMultiplier(cartindex, ::Opm::FaceDir::ZMinus); | ||
| }; | ||
| bool mergeMinPVCells = edge_conformal;// try to make geometrically connected grids |
There was a problem hiding this comment.
All the changes in this file seem unrelated. Maybe these should be in another PR?
2 participants
-- This fix some issues with mult pv when collapsed cells get a eps volume.