-
Notifications
You must be signed in to change notification settings - Fork 130
Rework initialization of segment pressures #6587
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
base: master
Are you sure you want to change the base?
Rework initialization of segment pressures #6587
Conversation
|
jenkins build this failure_report please |
| auto& new_well = this->well(w); | ||
| new_well.init_timestep(prev_well); | ||
|
|
||
| if (prev_well.status == Well::Status::SHUT) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussion with @GitPaean, it appears that this does not trigger (since the well is not supposed to copy the shut-well state)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change in this PR is going through a different path.
Opm::WellState::initWellStateMSWell WellState.cpp:692
Opm::BlackoilWellModel::initializeLocalWellStructure BlackoilWellModel_impl.hpp:269
Opm::BlackoilWellModel::beginReportStep BlackoilWellModel_impl.hpp:208
Opm::SimulatorFullyImplicitBlackoil::runStep SimulatorFullyImplicitBlackoil.hpp:389
Opm::SimulatorFullyImplicitBlackoil::run SimulatorFullyImplicitBlackoil.hpp:187
Opm::FlowMain::runSimulatorRunCallback_ FlowMain.hpp:377
Opm::FlowMain::runSimulatorInitOrRun_ FlowMain.hpp:461
Opm::FlowMain::runSimulator FlowMain.hpp:364
Opm::FlowMain::execute_ FlowMain.hpp:257
Opm::FlowMain::execute FlowMain.hpp:176
Opm::flowMain<…> Main.hpp:92
Opm::Main::dispatchStatic_<…> Main.hpp:381
Opm::Main::runStatic<…> Main.hpp:158
Opm::flowBlackoilTpfaMainStandalone flow_blackoil.cpp:77
main flow_blackoil_main.cpp:24
__libc_start_call_main 0x00007ffff742a1ca
__libc_start_main_impl 0x00007ffff742a28b
_start 0x00005555556a9065
if (this->param_.use_multisegment_well_ && this->anyMSWellOpenLocal()) {
this->wellState().initWellStateMSWell(this->wells_ecl_, &this->prevWellState());
}But in the function this->wellState().initWellStateMSWell() it does not copy results from SHUT well either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is true. I'll need some more debugging to figure why an opened well can get initialized with zero bhp. The PR prevents this problem, but still it's likely a bug so worth figuring out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some more digging into this. Seems the issue is related to wells that get opened by an action. For the case I'm looking at, some shut wells get a WELOPEN (through pyaction). When next time-step begins, they go through initWellStateMSWell, but in contrast to wells that open normally, their segment rates/pressures are reset to previous state since prev_ws.status == Well::Status::SHUT returns false for these wells.
I don't really know the action-part of the code well, but could the reason for the failed logic above be the this->wellModel_.commitWGState() in applyActions?
We get around the above problem with this PR (will update), but could there be other parts of the code which relies on the same logic?
|
I have not looked at the code. Most of the regression failures are okay, while not sure what is the following failure about. is the following the reason that the test failed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the idea here, to have the segment pressure around the reservoir level so they can also produce, while totally sure how it should be done.
leaving some comments on the code level for now. more thinking are needed for the review.
opm/simulators/wells/WellState.cpp
Outdated
| // pressures equal to the closest set pressure in the outlet direction. | ||
| if (std::any_of(segment_pressures.begin(), segment_pressures.end(), | ||
| [](const Scalar p){ return p < 0.0; })) { | ||
| setSegmentPressuresFromOutlets(segment_pressures, segment_inlets, 0 /* segment */, -1 /* outlet segment */); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to make this code work segment_pressures[0] can not be negative. an safety checking will be needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if top segment pressure us unset at this stage, it means the well has no connections which I guess should not happen at this stage, but better to be safe. In this situation arises, I guess it's best just to set it to bhp (which means all segments will get that pressure).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or more directly, inside the function setSegmentPressuresFromOutlets(), make sure the -1 index should not be used.
opm/simulators/wells/WellState.hpp
Outdated
| const int segment, | ||
| std::vector<Scalar>& segment_rates); | ||
|
|
||
| void setSegmentPressuresFromInlets(std::vector<Scalar>& segment_pressures, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two functions have to be called together. Calling either one only is not a complete process.
They should be made private, and called through another function (can also be private unless needs to be public) including these two function calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will set them private. I'm not 100% sure the second one is needed (that is, if a well is allowed to have toe-segemnts without perforations), but included it just in case.
The local running with the PR is fine. Not totally sure what happens with Jenkins here. We will see from next jenkins test. |
|
jenkins build this failure_report please |
The failure was gone in the new jenkins result. |
|
jenkins build this failure_report please |
Current initialization of segment pressures can be problematic for two reasons:
ws.bhpis zero. If such a well subsequently opens, it will copy the segment pressures for the shut-state initialization, i.e., it gets zero pressure for all segments that does not have perforations. This situation leads to the well failing to produce non-zero rates incomputeWellRatesWithBhp(for any bhp > 0) as used in e.g.,initializeProducerWellState. Hence , such a well will fail to obtain explicit rates as needed in e.g., operability checks (in addition it's a difficult initial guess for the well solve).This PR simply sugests to use perforation pressures to initialize all segment pressures (and disregard the previously set
ws.bhp). In this way, in subsequent call toscaleSegmentPressuresWithBhp, all segment pressures will be approximately at level without large jumps, andcomputeWellRatesWithBhpwill not fail to produce non-zero rates (unless it should due to low reservoir pressure).