-
Notifications
You must be signed in to change notification settings - Fork 130
Fixes for gliftopt item 3 maximum gas+alq #6620
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
Conversation
|
jenkins build this failure_report please |
|
@totto82 I see you ran jenkins with "failure_report". Where can I view this report? Or maybe there is no report when all tests pass? |
|
@totto82 Is there any test case that will trigger the GLIFTOPT item 3 fix? |
|
no failures, no report. |
|
I added a new commit where a similar limiting of the gas phase is done during the optimization. |
|
jenkins build this failure_report please |
1 similar comment
|
jenkins build this failure_report please |
I have extended the glift1 test to test this explicitly. |
| num_wells_changed = simulator.vanguard().gridView().comm().sum(num_wells_changed); | ||
|
|
||
| if (num_wells_changed > 0) { | ||
| updateWellPotentials(simulator, well_container, node_pressures, wellState, deferred_logger); |
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.
This will only updates potentials for network nodes, right? Maybe add a comment about why potentials for other nodes are not updated? And why all network nodes need to be updated (and not only those that was changed)
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 have removed this and instead store the well potentials during gaslift to avoid re-computing the well potentials again.
|
|
||
| if (controls.hasControl(Well::ProducerCMode::ORAT) && oil_rate > static_cast<Scalar>(controls.oil_rate)) { | ||
| water_rate *= (static_cast<Scalar>(controls.oil_rate) / oil_rate); | ||
| gas_rate *= (static_cast<Scalar>(controls.oil_rate) / oil_rate); |
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.
- It seems unlikely that
oil_ratewill be zero since it is known to be greater thancontrols.oil_rate, but ifcontrols.oil_rate ~= 0it could in theory become very small. Should we check for this before scaling? 2) We could also add a comment about why this scaling is needed.
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.
Since oil_rate > controls.oil_rate >= 0 I think controls.oil_rate / oil_rate should be safe.
| const int gas_pos = pu.canonicalToActivePhaseIdx(IndexTraits::gasPhaseIdx); | ||
| well_pot[gas_pos] = state->gasRate(); | ||
| const bool isThp = (ws.production_cmode == Well::ProducerCMode::THP); | ||
| if (isThp) { |
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.
Please add a comment why we only want to update potentials for THP controlled wells
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 gasRate() from state is only equal the potential for THP wells. With the update in the last commit we now store the well potential as well as the rate to make sure we can update the well potential for all wells during gaslift optimization.
| alq_opt = std::nullopt; | ||
|
|
||
| return {alq_opt, limited}; | ||
| return {alq_opt, limited && increase }; |
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 do not understand why we need && increase 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.
I added a comment
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.
This PR looks very good. The added test cases and will definitely be very valuable in the future. I will approve this after the comments have been addressed.
|
jenkins build this failure_report please |
| if (limited && checkALQequal_(orig_alq, alq)) | ||
| alq_opt = std::nullopt; | ||
|
|
||
| // alq_is_limited is used to check if we can increase the alq or not |
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.
alq_is_limited is used to check if we can increase the alq or not
Isn't it also used to check if we can decrease ALQ?
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 way I read checkRateAlreadyLimited_ where isAlqIsLimited() is used, it only do_check if increase is true. But maybe I missed something. I can do some more checking.
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.
If both *current_increase and increase are false, see line 197 it should do the check, right?
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.
You are, of course, right. With the current code, we may be in a situation where alq_is_limited = true and alq = alq_max, but we still want to be able to decrease it. I will provide a proper fix.
|
jenkins build this failure_report please |
|
jenkins build this failure_report please |
|
Thanks for the reviews. |
No description provided.