Avoid hidden dual state pattern in solveWellWithZeroRate() #6631
Conversation
The solveWellWithZeroRate() method created an empty group state and pushed it into the wgHelper object, then passed wgHelper (along with the simulator object) to iterateWellEqWithSwitching(). Further down the call chain in wellUnderZeroGroupRateTarget(), the real group state was recovered by accessing the simulator object, defeating the convention that wgHelper should be the single source of truth for the current group state. solveWellWithZeroRate() needs to use both an empty group state when assembling the control equation and the real group state when querying the well's actual operational status in wellUnderZeroGroupRateTarget() (to check whether its parent group has a zero rate target). We solve this by passing a flag "solving_with_zero_rate" down from solveWellWithZeroRate() and letting the assembleWellEqWithoutIterationImpl() method check the flag and create the empty group state locally when the flag is set. This eliminates the need for pushing/popping group state in wgHelper while maintaining the distinction between assembly (uses empty group state) and queries (uses real group state). The issue of having both simulator and wgHelper as sources for the real group state will be addressed in a follow-up PR.
hakonhagland
added
the
manual:irrelevant
|
jenkins build this please |
atgeirr
left a comment
atgeirr
left a comment
There was a problem hiding this comment.
Good detective work, should eventually be merged.
Open to discuss the default arguments, if there are very good reasons we need them (for now) I am willing to let that pass.
| DeferredLogger& deferred_logger, | ||
| const bool fixed_control = false, | ||
| const bool fixed_status = false) = 0; | ||
| const bool fixed_status = false, |
There was a problem hiding this comment.
Default arguments are not great, although they can help in the short term to avoid dealing with interface changes. I would like to avoid it when possible though! Is it feasible to remove the default arguments from this (virtual function) overload at least? It seems to me it is primarily called from the non-virtual overload and not from very many places.
If possible, we should also consider updateWellControlAndStatusLocalIteration(), but I assume the function here is easiest to remove defaults from.
| if (!fixed_control) { | ||
| // When solving_with_zero_rate=true, fixed_control=true, so this block should never | ||
| // be entered. | ||
| assert(!solving_with_zero_rate); |
There was a problem hiding this comment.
I think assert() is best used when the logic that is assert()ed is contained within the function itself. Here it is an assumption on the function interface, for a function that does not even have any docs. While I think the check should remain, I prefer that it is an exception since the logic it checks must ensured on the function call site.
We have already changed many such assertations in the code to exceptions.
Default arguments have maintenance issues so avoid using them if possible.
Convert assert() to throw to be consistent with usage elsewhere in the code base where external interface violations should cause thrown exceptions instead of causing assertion failures which will not even be enforced in release builds.
|
jenkins build this please |
|
Thanks for the improvements, merging! |
2 participants
The solveWellWithZeroRate() method creates an empty group state and pushes it into the
wgHelperobject, then passeswgHelper(along with thesimulatorobject) toiterateWellEqWithSwitching(). Further down the call chain inwellUnderZeroGroupRateTarget(), the real group state is recovered by accessing thesimulatorobject, defeating the convention thatwgHelpershould be the single source of truth for the current group state.So
solveWellWithZeroRate()needs to use both an empty group state when assembling the control equation and the real group state when querying the well's actual operational status inwellUnderZeroGroupRateTarget()(to check whether its parent group has a zero rate target).We solve this requirement by passing a flag
solving_with_zero_ratedown fromsolveWellWithZeroRate()and letting theassembleWellEqWithoutIterationImpl()method check the flag and create the empty group state locally when the flag is set. This eliminates the need for pushing/popping group state inwgHelperwhile maintaining the distinction between assembly (uses empty group state) and queries (uses real group state).The issue of having both
simulatorandwgHelperas sources for the real group state will be addressed in a follow-up PR.This issue with dual state opened up for the bug and jenkins failure in #6626. That PR tried to eliminate the
simulatorobject inwellUnderZeroGroupRateTarget()and thereby accidentally using the empty group state in the wgHelper object instead, see WellInterface_impl.hpp:1692When this PR is merged, the plan is to rebase #6626 and fix the bug there.