-
Notifications
You must be signed in to change notification settings - Fork 123
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
Implement gsatprod #5894
Implement gsatprod #5894
Conversation
Now also with interaction with group controls. |
Simple test case added. OPM/opm-tests#1286 |
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 looks okay to me, but I really think we should try to limit the amount of nested logic we add when making new features, and, generally try to simplify the functions if at all possible. The WellGroupHelpers
are already quite complicated and the fact that we can't understand any single function in isolation because everything gets accumulated into updateAndCommunicateGroupData()
.
const auto& gsatprod = schedule[reportStepIdx].gsatprod.get(); | ||
// only sum once | ||
if (wellState.isRank0() && !injector && gsatprod.has(groupName)) { | ||
const auto& gsatprod_rates = gsatprod.get(groupName); | ||
const auto& pu = wellState.phaseUsage(); | ||
using Rate = GSatProd::GSatProdGroup::Rate; | ||
if (pu.phase_used[BlackoilPhases::Aqua] && pu.phase_pos[BlackoilPhases::Aqua] == phasePos) { | ||
rate += gsatprod_rates.rate[Rate::Water]; | ||
} | ||
if (pu.phase_used[BlackoilPhases::Liquid] && pu.phase_pos[BlackoilPhases::Liquid] == phasePos) { | ||
rate += gsatprod_rates.rate[Rate::Oil]; | ||
} | ||
if (pu.phase_used[BlackoilPhases::Vapour] && pu.phase_pos[BlackoilPhases::Vapour] == phasePos) { | ||
rate += gsatprod_rates.rate[Rate::Gas]; | ||
} | ||
} |
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 think I'd prefer if this logic were separated out to a distinct helper function, or at least a separate loop. Something along the lines of
template <typename Scalar>
Scalar satelliteProduction(const ScheduleState& sched,
const std::vector<std::string>& groups,
const GSatProd::GSatProdGroup::Rate rateComp)
{
auto satProdRate = Scalar{};
const auto& gsatProd = sched.gsatprod();
for (const auto& group : groups) {
if (! gsatProd.has(group)) {
continue;
}
gsatProdRate += gsatProd.get(group).rate[rateComp];
}
return gsatProdRate;
}
std::optional<GSatProd::GSatProdGroup::Rate>
selectRateComponent(const PhaseUsage& pu, const int phasePos)
{
using Rate = GSatProd::GSatProdGroup::Rate;
for (const auto& [phase, rateComp] : std::array {
std::pair { BlackoilPhases::Aqua, Rate::Water },
std::pair { BlackoilPhases::Liquid, Rate::Oil },
std::pair { BlackoilPhases::Vapour, Rate::Gas } })
{
if (pu.phase_used[phase] && (pu.phase_pos[phase] == phasePos)) {
return rateComp;
}
}
return std::nullopt;
}
which you'd call, either as part of sumWellPhaseRates()
or separately, as
if (wellState.isRank0() && !injector) {
const auto rateComp = selecteRateComponent(wellState.phaseUsage(), phasePos);
if (rateComp.has_value()) {
rate += satelliteProduction(schedule[reportStepIdx], group.groups(), *rateComp);
}
}
This would reduce the amount of nested logic.
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 suggestions.
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.
Thank you for the updates. I think this is starting to look good. Just one small issue remaining, I think.
// only sum satelite production once | ||
if (wellState.isRank0() && !injector) { | ||
const auto rateComp = selectRateComponent(wellState.phaseUsage(), phasePos); | ||
if (rateComp.has_value()) { | ||
rate += satelliteProduction(schedule[reportStepIdx], group.groups(), *rateComp); | ||
} | ||
} |
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 section needs to be outside of the groupName
loop lest we include the contributions multiple times in the case that more than one of group.groups()
is a satellite.
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 a lot for the updates. This looks good to me now and I'll merge into master.
Depends on
OPM/opm-common#4188
Sufficient to run OPM/opm-tests#1207 where GSATPROD only interact with output and new test case OPM/opm-tests#1286