-
Notifications
You must be signed in to change notification settings - Fork 122
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
Allow group guide rates in case that a group is an auto choke group #5754
base: master
Are you sure you want to change the base?
Conversation
jenkins build this opm-common=4355 please |
jenkins build this opm-common=4355 please |
jenkins build this opm-common=4355 please |
1 similar comment
jenkins build this opm-common=4355 please |
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.
Thanks for the contribution. I have done the first review. I still struggle to understand the complete logic here, but for what I understand the code looks ok. My main concern is the code duplication in getAutoChokeGroupProductionTargetRate. This code is complicated so for maintenance it would be good to reduce the places the logic is copied. Also, I am not fully convinced that using guiderates from the production control is always ok. What if it is defaulted? Shouldn't it then accumulate it from its children group/wells?
} | ||
|
||
const Scalar orig_target = target_tmp; | ||
std::cout<< "Group: " << group.name() << " orig_target: " << orig_target*86400 << std::endl; | ||
|
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.
Why do you introduce a target_tmp in addition to orig_target here?
@@ -147,7 +150,7 @@ guideRate(const std::string& name, | |||
return WellGroupHelpers<Scalar>::getGuideRate(name, schedule_, well_state_, group_state_, | |||
report_step_, guide_rate_, target_, pu_); | |||
} else { | |||
if (groupControlledWells(name, always_included_child) > 0) { | |||
if ((groupControlledWells(name, always_included_child) > 0)) { | |||
if (is_producer_ && guide_rate_->has(name)) { |
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.
why the extra parentheses
const auto& control_group = schedule.getGroup(control_group_name, report_step); | ||
const auto& ctrl = control_group.productionControls(summary_state); | ||
const auto& control_group_guide_rate = ctrl.guide_rate; | ||
const auto& control_group_cmode = ctrl.cmode; |
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.
What happens if the control group does not have a guiderate?
const auto& control_group_cmode = ctrl.cmode; | ||
|
||
const auto& group_guide_rate = group.productionControls(summary_state).guide_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.
Same here for the group
const Scalar current_rate = tcalc.calcModeRateFromRates(rates); | ||
|
||
if (current_rate < 0.99 * target_rate) | ||
included = false; |
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.
Why do you need this 0.99 factor?
template<class Scalar> | ||
std::pair<Scalar, Group::ProductionCMode> WellGroupControls<Scalar>:: | ||
getAutoChokeGroupProductionTargetRate(const std::string& name, | ||
const Group& group, |
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.
What is the difference between this code and getGroupProductionTargetRate? Can this be refactored to avoid code duplication.
This PR is to allow group guide rates in case that a group is an auto choke group:
In the example case we have:
and
The
M5S
group is an ancestor of the production groupsB1
andC1
andB1
is an autochoke group ( #4935). The target rates of B1 and C1 become7000 (=8000*35/40)
and1000 (=8000*5/40)
A comparison with a reference simulator is shown in the figure
Note that OPM-Flow keeps wells open for a longer time.
The test case is: GROUPGUIDERATES_AC_8000_OPM_TEST.zip