-
Notifications
You must be signed in to change notification settings - Fork 123
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
Add support for directional relative permeabilities #4048
Conversation
Depends on OPM/opm-models#714, OPM/opm-material#532, and OPM/opm-common#3113. Implements support for directional relative permeabilities ( |
jenkins build this opm-models=714 opm-material=532 opm-common=3113 please |
1 similar comment
jenkins build this opm-models=714 opm-material=532 opm-common=3113 please |
Thanks for the contribution. I think the opm-common/opm-material parts looks good. For opm-models my main concern is that this adds mobility[X,Y,Z] to the blackoilintensivequantities even in cases where KRNUMX, KRNUMY, KRNUMZ is not used. Since directional relative permeability is typically not very often used a solution with conditional storage of the extra mobiles is better IMO. For the opm-simulator part the NNC issue (https://ci.opm-project.org/job/opm-simulators-PR-builder/3916/testReport/junit/(root)/mpi/compareECLFiles_flow_3D_2AQU_NUM/) needs to be resolved. Maybe just using the relperm from the original satnum region will do for now. This implementation keeps the mobility call without the faceID. Is this a temporary fix to avoid changes in the output code etc. |
@totto82 Good idea, I have updated this PR (and OPM/opm-material#532 and OPM/opm-models#714) with a new approach that only has an empty shared pointer if directional permeabilities are not used.
Yes indeed! In this update I try to check if directional relative permeabilties are enabled before trying to access the faceid. Then, only if the user enables directional relperms and also uses a grid with NNC faces the exception will be thrown. See also the corresponding update in OPM/opm-models#714 |
jenkins build this opm-models=714 opm-material=532 opm-common=3113 please |
I have looked at this, and note that as written, this is not compatible with the TpfaLinearizer fast assembly. If directional relperms are needed on real fields we must do this in a way that is compatible. I think we may do this by adding a face direction argument to the local residual's computeFlux() method, so it should not be too hard. The current way depends on changes to the stencil inside the element context, which is not used by the TpfaLinearizer. I think we should not merge until we have addressed this. |
@atgeirr You mean here https://github.com/OPM/opm-models/blob/2e2961939c181d87a9a0afaafac3e45eaffdb436/opm/models/blackoil/blackoillocalresidualtpfa.hh#L194 Why didn't Jenkins complain? I though we had tests for the TpfaLinearizer assembly. |
We do, but only for compilation at this point. And we have no tests that check that directional relative permeability works (of course...), that should be added. If this had been already merged, and we had an integration test for the functionality, then #4017 would have had that test failing. |
Only access the grid face direction information if directional relperms have been enabled.
Simplify code using the unknown facedir value.
@atgeirr Thanks for the comment! Yes I agree, we should add support for directional relative permeability for the flux computations in the tpfa linearizer also. I have updated this PR to address this, see also the corresponding update in OPM/opm-models#714 and OPM/opm-models#714. |
jenkins build this opm-models=714 opm-material=532 opm-common=3113 please |
To avoid compiler warnings, we also need to check for the unknown face direction due to change in the FaceDir::DirEnum values in opm-common.
Use the new name of the dirId() function in ecfvstencil.hh
jenkins build this opm-models=714 opm-material=532 opm-common=3113 please |
Just for your information: Due to a quirk of the CI system, you should generally trigger multi-repository builds from the most upstream repository—in this case OPM-Common. The reason is that unit tests in upstream repositories won't run otherwise. |
@bska Thanks for the information. I have triggered a new jenkins build in OPM/opm-common#3113 instead. |
This is used by blackoilintensivequantities.hh. Further, for other problems derived from MultiPhaseBaseProblem, that base class will return a null pointer such that problems that do not override the materialLawManagerPtr() method still can use the blackoil intensive quantitites (which in the case of directional relative permeabilities makes use of the problem reference to access the materialLawManager)
No description provided.