OPM
Check status from wellstate not the well object from Schedule
I think the failures for WCYCLE is expected. As I anticipated in https://github.com/OPM/opm-simulators/pull/6050 the current code in master may not by 100 correct for WCYCLE. @akva2 I would appreciate if you could take a look and see if you agree.
Firstly, the wcycle failures here are entirely trivial and indistinguishable to the eyeball norm. The min_bhp one is more severe (but in the correct direction) as some spikes disappeared there. As far as correctness of WCYCLE, I mostly did what monkey saw others monkey do, so it is entirely possible I have checked the wrong status variable. I assume you are not on the monkey level, monkey will trust big fire god.
@bska Do you have any further comments on this PR? If we agree that this is more consistent I would like to merge this to as a preparation for https://github.com/OPM/opm-simulators/pull/6050
We are checking the "dynamicStatus" of the well in the summary output code. Currently, this is updated in assignShutConnections()
xwel.dynamicStatus = this->wellState().well(well.name()).status;
I would like to use this feature when handling item 9 in WECON. See https://github.com/OPM/opm-simulators/pull/6050
Does it make sense to move this out from this function (maybe to a separate assignDynamicWellStatus()) since it is not only relevant for assignShutConnections()
I would like to use this feature when handling item 9 in WECON. See #6050.
Right. Function assignShutConnections() is supposed to leave (the connection information of) EDIT:open connections in flowing wells untouched, so if the well is currently flowing as determined by SingleWellState::status or some other authoritative source, then this function should do nothing.
So far so good. The problem comes for wells which are not flowing. Most of the logic in assignShutConnections() exists to handle the case of a well which is dynamically shut in, but which is supposed to be flowing according to the simulation input, i.e., in the SCHEDULE section. When we added this function, the only case in which a well would be recorded as non-flowing was in the context of well testing (WTEST keyword). That's why it assigns the dynamicStatus based on wellTestState(). Furthermore, the condition concerning wasDynamicallyShutThisTimeStep() is because we're supposed to be reporting the values as if the well is flowing at the end of the time step at which the well is shut in.
Finally, the skip() helper function is intended to ignore open connections in flowing wells while ensuring that the $Kh$ product, the CTF, and the connection $D$ factor are reported as their input values for shut connections in flowing wells, and all connections in shut-in wells. I'd really like to keep that behaviour.
Thank you for the explanation. I think the implementation in this PR should align with your explanation, but maybe I still misunderstood something. The test failures are related to WCYCLE, for which I think this PR gives an improvement.
I think the implementation in this PR should align with your explanation,
Almost. I think we still need something along the lines of !shutThisTimeStep() to ensure that we report flowing conditions at the end of a time step that shuts in a well. I assume that SingleWellState::status will be "stop" or "shut" in that case, but the dynamicState is supposed to be "open", at least for reporting purposes.
Please have a look at PR #6238. This part of the code changed recently in order to simplify defining data::Well::dynamicStatus.
@bska Thanks. I didn't notice. I assume I need to move my stuff to assignDynamicWellStatus
Also, I'll repeat what I tried to say earlier: There are two cases at play here, and your changes (this PR and #6050) seem very focused on one, while the existing code deals mostly with the other.
- A well is shut in the deck input, but opens dynamically as a result of
WECON(9) - A well is open in the deck input, but closes dynamically as a result of operability constraints and well testing
As far as I can tell, your changes are intended to deal with 1., but the existing code deals with 2. We should absolutely handle 1., but not at the expense of mishandling 2.
Did I get it right this time?
It Depends[tm]. Specifically, it depends on subtleties of definition. The OP_1 production well of the MIN_THP_1 is inoperable and closes (shuts in) due to "physical reasons" fairly early in the simulation run. However, it gets re-opened in WTEST 30 days after its initial shut-in. The well is still not operable and immediately shuts in, but the current master sources still reports the well flowing at its test point. This PR changes that behaviour and reports the well being shut from the initial shut-in point and through to the end of the simulation. As for what kind of reporting behaviour we actually want in this situation I don't really know. Perhaps @GitPaean could give some guidance?
Starting time step 15, stepsize 5 days, at day 624/640, date = 16-Mar-2020
well OP_1 is being tested
Newton its= 2, linearizations= 3 (0.0sec), linear its= 2 (0.0sec)
I assume you refer to this event. My understanding is that since the well testing was not successful (if successful, it should give a "is re-opened"....) we should not output it at all.
The well is still not operable and immediately shuts in, but the current master sources still reports the well flowing at its test point.
There has been some development/verification along this. Personally I think not reporting is fine/good if the re-opening is not successful, while there might be some observation that supports to output the values before it gets SHUT. Let me look into it. @steink , do you remember anything regarding this?
When looking at what exactly happens, it shows something not directly related to this PR, while show how the wells are tested.
The testing efforts failed because
while (testWell) {
const std::size_t original_number_closed_completions = welltest_state_temp.num_closed_completions();
bool converged = solveWellForTesting(simulator, well_state_copy, group_state, deferred_logger);
if (!converged) {
const auto msg = fmt::format("WTEST: Well {} is not solvable (physical)", this->name());
deferred_logger.debug(msg);
return;
}
did not manage to get converged.
And in the function solveWellForTesting() because well_state.well(this->index_of_well_).status is SHUT, it enters
the else condition branch.
if (this->param_.use_implicit_ipr_ && this->well_ecl_.isProducer() && this->wellHasTHPConstraints(summary_state) && (well_state.well(this->index_of_well_).status == WellStatus::OPEN)) {
converged = solveWellWithTHPConstraint(simulator, dt, inj_controls, prod_controls, well_state, group_state, deferred_logger);
} else {
converged = this->iterateWellEqWithSwitching(simulator, dt, inj_controls, prod_controls, well_state, group_state, deferred_logger);
}
And in the function iterateWellEqWithSwitching(), allow_switching will be false due to the following code,
const bool allow_open = well_state.well(this->index_of_well_).status == WellStatus::OPEN;
// don't allow switcing for wells under zero rate target or requested fixed status and control
const bool allow_switching =
!this->wellUnderZeroRateTarget(simulator, well_state, deferred_logger) &&
(!fixed_control || !fixed_status) && allow_open;
Then it tries to solve with allow_switching to be false and did not manage to get converged.
I can be wrong, but it looks like very worrying, since I believe solveWellForTesting() is probably to check whether the well is solvable at all with the least restrictive condition (e.g. BHP constraint).
@steink , any comments regarding this part?
@GitPaean Thanks for noticing. I think we should add a ws.open() to the well_state_copy before we do the well testing. I will update the PR accordingly to test.
@GitPaean Thanks for noticing. I think we should add a ws.open() to the well_state_copy before we do the well testing. I will update the PR accordingly to test.
Yeah. Now I see it is some difference introduced from this PR.
well_state.well(this->index_of_well_).status is SHUT
Are you sure about this? When I just tested, the status was OPEN at the point you refer to.
well_state.well(this->index_of_well_).status is SHUT
Are you sure about this? When I just tested, the status was OPEN at the point you refer to.
I think so, with this PR. And make sure you enter there through the well testing. And also --enable-tuning=true , that is how the regression is setup with TUNING.
With enable-tuning I was able to reproduce. I have added as ws.open(), which I think is good anyway.