Grid independent well specification #3384
Conversation
Reorganizing and renaming files
|
jenkins build this please |
|
jenkins build this please |
|
jenkins build this please |
|
jenkins build this please |
|
jenkins build this please |
|
jenkins build this please |
|
This PR relies on a large number of files of ResInsight that is placed under |
|
no globbing. |
Very clear :) .Going to list the files manually |
|
jenkins build this please |
There was a problem hiding this comment.
This is only a partial review, more to come. You will need to rebase on current master (and run into conflicts). Might be easier to use my version rebased on current master, see https://github.com/blattms/opm-common/tree/well-traj. Please yell if you need further instructions (e.g. for git reset).
It is still a lot of files from ResInsight and drags in quite some baggage (e.g. own smart pointer class, etc.) because of ResInsight being developed on Windows.
| @@ -20,6 +20,7 @@ | |||
| #define SCHEDULE_GRID | |||
|
|
|||
| #include <opm/input/eclipse/Schedule/CompletedCells.hpp> | |||
| #include <opm/input/eclipse/EclipseState/Grid/EclipseGrid.hpp> | |||
There was a problem hiding this comment.
Are you sure we need this include? There seems to be a forward declaration below. Maybe try to delete this line?
There was a problem hiding this comment.
The include file seems indeed not needed. I removed it.
| cvf::Vec3d calculateLengthInCell( const std::array<cvf::Vec3d, 8>& hexCorners, | ||
| const cvf::Vec3d& startPoint, | ||
| const cvf::Vec3d& endPoint ) const; | ||
|
|
||
| void findCellLocalXYZ( const std::array<cvf::Vec3d, 8>& hexCorners, | ||
| cvf::Vec3d& localXdirection, | ||
| cvf::Vec3d& localYdirection, | ||
| cvf::Vec3d& localZdirection ) const; |
There was a problem hiding this comment.
nitpick: Indentation looks a bit off. Maybe you are using tabs?
| @@ -189,6 +191,67 @@ namespace { | |||
| } | |||
| } | |||
|
|
|||
| void Schedule::handleWELTRAJ(HandlerContext& handlerContext) { | |||
| std::unordered_set<std::string> wells; | |||
There was a problem hiding this comment.
This does not really seem to be used besides adding well names to it. Is that intentional?
There was a problem hiding this comment.
It calls the method loadWELTRAJ that reads in the well trajectory data x,y,z and md (measured depth) provided in table WELTRAJ. This data is used in handleCOMPTRAJ together with data from table COMPTRAJ.
There was a problem hiding this comment.
Sorry for not being clear. I was talking about the local variable wells, and not this method. It gets populated but is not used for anything else.
There was a problem hiding this comment.
I get it now. This was not intentional. I removed it.
| if (defaultSatTable) | ||
| satTableId = props->satnum; | ||
|
|
||
| auto same_ijk = [&]( const Connection& c ) { |
There was a problem hiding this comment.
Maybe just capture what you need? Even by value should be fine ... = [I, J, K](...)
There was a problem hiding this comment.
This inline function returns a boolean and is used later in std::find_if. This construction is also there in loadCOMPDAT.
There was a problem hiding this comment.
I think there might be a misunderstanding. I was talking about [&], which captures all local variable by reference to make them usable inside your lambda. As you are just using I, j, and K, I was suggesting to only capturing them.
Anyway, this is probably not a big deal, and just me justifying my existence as a reviewer 😅. Hence feel free to leave as is.
There was a problem hiding this comment.
Ah I see, indeed I did misunderstood. I followed your suggestion.
src/opm/input/eclipse/Schedule/WellTraj/RigEclipseWellLogExtractor.cpp
Outdated
Show resolved
Hide resolved
…o well-traj-markus Use Markus' rebased version
There was a problem hiding this comment.
Concerning the [RigEclipseWellLogExtractor.* files: May I suggest to add some documentation to the methods to make it easier for collaborators to see what they are doing?
These files seem to also originate from ResInsight but already have diverged quite a bit when compared to the curretnt ResInsight version. Maybe we should add a comment to those files saying where they come from and from which version, and what was changed and why?
This completes my review. Please note that I have not reviewed anything under external.
src/opm/input/eclipse/Schedule/WellTraj/RigEclipseWellLogExtractor.cpp
Outdated
Show resolved
Hide resolved
src/opm/input/eclipse/Schedule/WellTraj/RigEclipseWellLogExtractor.cpp
Outdated
Show resolved
Hide resolved
| RigHexIntersectionTools::lineHexCellIntersection( p1, p2, hexCorners, globalCellIndex, &intersections ); | ||
| } | ||
|
|
||
| if ( !isCellFaceNormalsOut ) |
There was a problem hiding this comment.
t seems that this is alwys false because of line 53
| const cvf::Vec3d& endPoint ) const | ||
| { | ||
| const auto[i,j,k] = m_grid.getIJK(cellIndex); | ||
| cvf::Vec3d hexCorners_opm[8]; //opm numbering |
There was a problem hiding this comment.
See my previous suggestion. Should maybe become a const static class member even.
Might also be good to put this recurring translation into its own function for reuse.
| cvf::Vec3d crossPoductIZ; | ||
| crossPoductIZ.cross( faceCenterCenterVectorI, localZdirection ); | ||
| localYdirection = faceCenterCenterVectorJ - crossPoductIZ; | ||
| localYdirection.normalize(); |
There was a problem hiding this comment.
Somehow I have the feeling that calculating localYdircection is redundant, as both localXdirection and localZdirection are already perpendicular to faceCenterVectorJ.
I am not 100% sure what you aim to compute here and for what it is needed. But please be aware that depending on your faces localZdirection might be quite skewed (i.e. very different from the direction of the columns used to define the cornerpoint grid.
There was a problem hiding this comment.
The aim of calculating the local X,Y,Z axes of a cell is to project the well line segment onto these axes. Then for each projected line segment a Peaceman connection factor is calculated ( CFx, CFy and CFz) that is combined into a single connection factor CF = sqrt(CFx^2+CFy^2+CFz^2).
| cellIndicesForBoundingBoxes.insert( cellIndicesForBoundingBoxes.end(), | ||
| threadIndicesForBoundingBoxes.begin(), | ||
| threadIndicesForBoundingBoxes.end() ); |
There was a problem hiding this comment.
I don't understand this construction.
As threadIndicesForBoundingBoxes is declared outside of the for loop, never cleared, and entries are always added with emplace_back, I am assuming that cellIndicesForBoundingBoxes contains a lot of duplicates. I am even assuming that in the worst case it will contain cellCount*cellCount entries. Is this what we want? Or am I missing something here?
| threadIndicesForBoundingBoxes.begin(), | ||
| threadIndicesForBoundingBoxes.end() ); | ||
|
|
||
| cellBoundingBoxes.insert( cellBoundingBoxes.end(), threadBoundingBoxes.begin(), threadBoundingBoxes.end() ); |
There was a problem hiding this comment.
errh, and this one might suffer from the same problem.
I must be missing something.
There was a problem hiding this comment.
I did not touch upon these constructions of ResInsight but don't see it growing beyond cellCount size.
|
Concerning licenses. As resinsight has multiple licenses (LGPL-2.1, GPL 3+, and MIT), I think it might make sense to only choose only GPLv3+ for the stuff copied to opm-common to make life easier. Comments? |
|
Yes, I agree that GPLv3+ makes most sense here. Multiple licenses is confusing. |
|
jenkins build this please |
|
jenkins build this please |
|
Well done, I appreciate that this work is now merged. Thanks for your efforts, looking forward to see how users will work with the new funktionality. |
|
Thanks @blattms for your helpful review! |
The plan is to test the functionality in well trajectory optimization workflows and to extend it to mult-segment wells |
|
@magnesj I am curious: As opm-common is a dependency for ResInsight, do you intend to use the merged code within resinsight eventually? |
As we compile for Windows, we need to do several adjustments to source files in Here is the modified branch we use https://github.com/CeetronSolutions/opm-common/tree/windows-build-2022-01 |
The functionality of this PR allows to specify a geometrical (grid independent) well trajectory and completion data.
For that two new tables
WELTRAJandCOMPTRAJare introduced. Examples of such tables:An example case is
opm-common/tests/SPE1CASE1_WELTRAJ.DATAand unit test is added toConnectionTests.cpp.Most (if not all) suggestions of @OPMUSER (see #3321) are included. In
WELTRAJandCOMPTRAJtheBRANCHNOcolumn is added to anticipates on multi-lateralsPERFTOPandPERFBOTare top and bottom of the perforation.PERFREFcolumn is introduced to allow a choice of specifying perforations in terms ofMDor e.g.TVD. Currently onlyMDis implemented. Purpose ofCOMPLNOcolumn is to allow lumping of completions, this is not implemented. AlsoDFACT(non-Darcy factor for gas wells) is not implemented (is also not implemented forCOMPDAT)CONNFACTandKHare either both specified or both negative/defaulted. In the latter case the connection factorCF(andKh) is calculated according toCF = sqrt(CFx^2+CFy^2+CFz^2)withCFx,CFyandCFzthe Peaceman connection factors for the well line-segment in the grid cell composed in three projected line-segments on the local(x,y,z)coordinate system.Part of the code of Resinsight is re-used for the calculation of the
IJKcoordinates of intersected cells and the well line-segment. The resinsight code is put inexternal/resinsight. Some code modification was needed to remove some dependencies e.g. on the QT library.