obtaining the depth for the completions based on segment information

[GitPaean]

it corrects the depth information when more than one completions is related to one segment, basically it provides one more feature of multi-segment well.

[joakim-hove]

Test

[GitPaean]

Test

I have that in mind, while I need a reference result to verify the algorithm first. That is why it is marked as WIP for now.

[GitPaean]

This PR works for the only example we have that has more than one perforation related to one of the segments. I does not break any our hand-made example either. I do realize the algorithm can fail for some situation. For example, if the perforation is related one segment, while it is farther than the segment from the well head, should we use the segment it is related to to do extrapolation or based on the inlet segment of the segment owns the perforation to do interpolation to calculate the depth of the perforation.

It sounds confusing probably, while basically, it is something we need to create example to test with.

[GitPaean]

The PR is ready for review now. The test is updated.

[GitPaean]

jenkins build this please

[GitPaean]

jenkins build this please

[GitPaean]

jenkins build this please

[joakim-hove]

I must admit I find this code quite difficult to follow; some comments on the roles of the three classes Compsegs, Segment and SegmentSet would be valuable.

It is my impression that the Compsegs class is only used temporarily during the construction phase? If that is indeed the case I would suggest that Compsegs.hpp header file is located with source files - and not installed.

[joakim-hove]

It is good that you have added a test with an input deck, but I will not merge this before you have taken the full blue pill and written some basic instantion tests:

std::vector<CompSegs> compsegs;

// Add three segments ...
compsegs.emplace_back( i,j,k,..... ); 
compsegs.emplace_back( ... );
compsegs.emplace_back( ... );

// Create a segment set and a completion set
SegmentSet segments;
CompletionSet completions;
...

// Call the static process routines in the Compsegs class
Compsegs::processCOMPSEGS( compesgs, segments );
Compsegs::updateCompletionsWithSegment( compsegs, completions );

// Check the results - what is actually the expected outcome here?

[GitPaean]

I must admit I find this code quite difficult to follow

Yes. it was difficult to program too. Basically, the algorithm follows the result we obtain from running, and some part is not very logical.

[GitPaean]

It is my impression that the Compsegs class is only used temporarily during the construction phase? If that is indeed the case I would suggest that Compsegs.hpp header file is located with source files - and not installed.

Yes. Compseg is used to build the relation between completions and segments. Do you want me to move the Compsegs.hpp out from lib/eclipse/include folder to the lib/eclpse/EclipseState/...?

[joakim-hove]

Do you want me to move the Compsegs.hpp out from lib/eclipse/include folder to the lib/eclpse/EclipseState/...?

Yes.

[GitPaean]

I think I have addressed all the comments except the instance test. I will try to add it as soon as possible.

Please let me know if you have more comments. Thanks for the review on the weekend, which is very helpful.

[joakim-hove]

Yes. Compseg is used to build the relation between completions and segments.

OK - I have looked a bit more on this code. As I understand it the net effect of Compsegs implementation is to update a CompletionSet instance with segment information on the completion objects. My suggestion is then that the only public function should be something like:

CompletionSet segment_completions( const DeckKeyword& compsegs, const CompletionSet& input_completions, const SegmentSet& segments) {
     CompletionSet new_completions;
     ...
     return new_completions;
}

Here I have just created a brand new CompletionSet instead of mutating the existing set. Whether this is implemented with a structure like Compsegs will then just be an implementation detail. For the completions I would reccomend a "copy" constructor like:

Completion::Completion( const Completion& initial, int segment_nr, double center_depth) {

}

then the segment information can be attached at construction time instead of mutating the objects afterwards.

[GitPaean]

OK - I have looked a bit more on this code. As I understand it the net effect of Compsegs implementation is to update a CompletionSet instance with segment information on the completion objects. My suggestion is then that the only public function should be something like:

I addressed the second one. For the first one, it involves re-factoring the Compsegs struct. This PR is only supposed to add one more feature (although not very trivial in some sense). Can we address this in a separate PR specifically for this comment with more discussion? If it is acceptable to you, can we also add the instance test in that PR later? There were no instance test, although probably it was also due to me.

[joakim-hove]

This PR is only supposed to add one more feature (although not very trivial in some sense). Can we address this in a separate PR [...]

yes

If it is acceptable to you, can we also add the instance test in that PR later?

No

[GitPaean]

Do you want me to move the Compsegs.hpp out from lib/eclipse/include folder to the lib/eclpse/EclipseState/...?

Yes.

Any suggestion how to include this header file in a tests now?

[joakim-hove]

Any suggestion how to include this header file in a tests now?

Well; welcome to the world of testing. The Compsegs structure is an internal detail - what you really want to test is only the ability to add segment information to completions - the difficulties you are now facing clearly tell you exactly this.

Maybe you should create a very small public segment structure which has only segment_nr and segment_depth - or maybe just a std::map<int,double> - after all that is all the interesting information extracted from the COMPSEGS keyword - and thereby also what the testing should focus on.

[GitPaean]

Well; welcome to the world of testing. The Compsegs structure is an internal detail - what you really want to test is only the ability to add segment information to completions - the difficulties you are now facing clearly tell you this.

But basically this test will not test the functionality introduces in this PR, which makes adding this test is to fix the old missing stuff.

Should I move this header file back to the include folder, which probably looks ugly to you? Any other suggestion?

[joakim-hove]

Should I move this header file back to the include folder, which probably looks ugly to you? Any other suggestion?

My suggestion:

• Make a public header in the include hierarchy Compsegs.hpp which has only one exported symbol:

  std::map<int,double> parseCOMPSEGS( const DeckKeyword& kw);

  which will parse a COMPSEGS keyword and return a sgement_nr / segment_depth mapping. How that function is implemented - is a detail; but I gase basing it on the now private (as entirely in the .cpp file) Compsegs class makes sense.

[atgeirr]

I think we got it now, we'll make a public header with a single function that will be tested and also used from handleCOMPSEGS().

[joakim-hove]

I think we got it now, we'll make a public header with a single function that will be tested and also used from handleCOMPSEGS().

Yes - that was my suggestion.

[GitPaean]

Another question, what is the best way to create a Keyword like COMPSEGS? Do we include an example anywhere? Thanks.

[GitPaean]

Is parseString the correct thing to do in this test to create the Keyword object?

[joakim-hove]

Is parseString the correct thing to do in this test to create the Keyword object?

yes

[GitPaean]

Thanks.

What the naming style requirement for a pure function header?

Should I capitalize updatingCompletionsWithSegments.hpp ?

[atgeirr]

Should I capitalize updatingCompletionsWithSegments.hpp ?

In opm-core and opm-simulators headers for functions have not been capitalized, just as you have done. For opm-parser I do not know, but I assume that is also ok there?

[joakim-hove]

For opm-parser I do not know, but I assume that is also ok there?

Sure.

[GitPaean]

I added a public header file and added a test. Please review.

[GitPaean]

It break the downstream compilation due to the interface in SegmentSet, which will be addressed in OPM/opm-simulators#1279.

[atgeirr]

It break the downstream compilation due to the interface in SegmentSet, which will be addressed in OPM/opm-simulators#1279.

When updates to that PR have been pushed we'll make a final Jenkins run for that PR which should include this.

@joakim-hove Do you have further requirements for this PR, or can we merge this when Jenkins is happy?

[GitPaean]

OPM/opm-simulators#1279 has been updated to incorporate the interface change from this PR and waiting for the Jenkis result.

Local building and running are normal.

[joakim-hove]

@joakim-hove Do you have further requirements for this PR, or can we merge this when Jenkins is happy?

Thank you for the effort - yes this can be merged now. I will let you do it when appropriate.

[joakim-hove]

yes this can be merged now.

Cool if you squash it on the way in.

[GitPaean]

Now, the only public interface related to COMPSEGS keyword is the updatingCompletionsWithSegments() . Please let me know if there is anything more is required to improve the COMPSEGS related and the test, I would like to address them. @joakim-hove .