Updating AICD/SICD scaling factors after parsing the whole report step#4803
Conversation
GitPaean
added
the
manual:enhancement
|
jenkins build this please |
GitPaean
force-pushed
the
updating_wsegsicd_wsegaicd
branch
2 times, most recently
from
cc9acfd to
9aa6175
Compare
|
I am marking the PR as ready for review. |
it is allowed to specify COMPSEGS after the WSEGACID/WSEGSICD keywords
GitPaean
force-pushed
the
updating_wsegsicd_wsegaicd
branch
from
9aa6175 to
c1ebbcd
Compare
|
The tests in MultisegmentWellTests.cpp are relatively simple and not touching the report steps, so the PR does not affect the tests there. If it is desirable to extend the tests there, please let me know. |
|
jenkins build this please |
I added a test in MultisegmentWellTests.cpp . |
|
jenkins build this please |
in MultisegmentWellTests
GitPaean
force-pushed
the
updating_wsegsicd_wsegaicd
branch
from
01eccd7 to
09d78f2
Compare
|
jenkins build this please |
daavid00
left a comment
daavid00
left a comment
There was a problem hiding this comment.
Thanks for this, I have tested in those decks in model 3 and now they run. Just a few nitpicks.
| void updateAutoICD(const AutoICD& aicd); | ||
| void updateValve(const Valve& valve, const double segment_length); | ||
| void updateValve(const Valve& valve); | ||
| bool updateICDScalingFactor(const double outllet_segment_length, const double completion_length); |
| const int segment_number = segment.segmentNumber(); | ||
| const auto outlet_segment = this->getFromSegmentNumber(segment_number).outletSegment(); | ||
| const auto outlet_segment_length = this->segmentLength(outlet_segment); | ||
| const auto completion_length = connections.segment_perf_length(segment_number);; |
| auto& sched_state = this->snapshots.back(); | ||
| for (const auto& well : sched_state.wells()) { | ||
| if (!well.get().isMultiSegment()) { | ||
| continue;; |
Thanks for the comments. I have updated the PR accordingly. |
|
jenkins build this please |
bska
left a comment
bska
left a comment
There was a problem hiding this comment.
Much appreciated. Other than the top-level Schedule::updateICDScalingFactors() function, this looks good to me.
| } | ||
| auto new_well{well.get()}; | ||
| if (new_well.updateICDFlowScalingFactors()) { | ||
| sched_state.wells.update( std::move(new_well) ); |
There was a problem hiding this comment.
This seems a little scary to me. You're effectively updating a structure over which you're iterating a sequence of references. I think I'd probably make this a two-stage process instead
auto updatedWells = std::vector<Well>{};
for (const auto& wellPair : sched_state.wells) { // Note: .wells rather than .wells()
if (! wellPair.second->isMultiSegment()) { continue; }
auto new_well = *wellPair.second;
if (new_well.updateICDFlowScalingFactors()) {
updatedWells.push_back(std::move(new_wells));
}
}
for (auto&& updatedWell : updatedWells) {
sched_state.wells.update(std::move(updatedWell));
}There was a problem hiding this comment.
Thanks. I thought some while somehow decided to go ahead. Will update.
GitPaean
force-pushed
the
updating_wsegsicd_wsegaicd
branch
from
b4029c6 to
ffa856e
Compare
|
jenkins build this please |
GitPaean
added
manual:bugfix
bska
left a comment
bska
left a comment
There was a problem hiding this comment.
Thanks a lot for the updates. This looks good to me now and I'll merge into master.
3 participants
it is allowed to specify COMPSEGS after the WSEGACID/WSEGSICD keywords.
following the testing through OPM/opm-tests#1413 .
This PR is another example that affects how the OPM-flow parse keywords. The SCHEDULE is very flexible, any keywords can be modified by a later input. There might be more situations that we have to use the final specification within the report step.