Check if a group has a choke #4622
Conversation
| GTNode groupTree(std::size_t report_step) const; | ||
| GTNode groupTree(const std::string& root_node, std::size_t report_step) const; | ||
| const Group& getGroup(const std::string& groupName, std::size_t timeStep) const; | ||
| bool hasChoke(const std::string& groupName, std::size_t timeStep) const; |
There was a problem hiding this comment.
I'd probably use a different parameter name here, since the predicate typically applies to a node named in the (extended) network. While groups may indeed be network nodes, not all network nodes are expected to be groups in this case.
Maybe nodeName instead of groupName?
There was a problem hiding this comment.
Thanks @bska. I was trying to fix the label but could not change it myself which I should be able to do, right?
There was a problem hiding this comment.
I was trying to fix the label but could not change it myself which I should be able to do, right?
You need "triage" privileges in the repository in order to set/change PR labels. At the moment, "triage" is effectively the same as "administrator" so only repository administrators have that ability. It's possible that we'll revise the PR labelling policy in light of this fact.
There was a problem hiding this comment.
I'd probably use a different parameter name here, since the predicate typically applies to a node named in the (extended) network. While groups may indeed be network nodes, not all network nodes are expected to be groups in this case.
Maybe
nodeNameinstead ofgroupName?
From the downstream PR, it looks like the function is to check whether a group is also a choke node in a network. Maybe groupName can be more appropriate as the input argument? But I also understand your arguments, so there is no strong objection to your suggestion either. Just for discussion.
There was a problem hiding this comment.
Whether or not something is a choke is inherently a node property, since this is governed by item 3 of the NODEPROP keyword. It's very misleading to give the impression that this pertains only to groups.
bska
added
the
manual:irrelevant
|
On a tangential note, we might want to add a unit test for this new function, both for the case of a node/group having a choke and for the case of there not being a choke. |
|
Hi, is the function used in some code already? Based on the usage, the design of the function can be some different. It is good to show the usage of the function in my opinion. |
Added a unit test for this |
I am preparing a PR in simulators in which I make use of it. |
|
jenkins build this please |
|
jenkins build this opm-simulators=6313 please |
Thanks. I now see the downstream PR now. OPM/opm-simulators#6313 From the usage, it looks like it is checking whether an certain group is also serving as a network node with an active choke setup. Maybe a more descriptive function name can be
|
|
With more thinking, it is okay to keep the function has_choke(), the parameter can be |
| GTNode groupTree(std::size_t report_step) const; | ||
| GTNode groupTree(const std::string& root_node, std::size_t report_step) const; | ||
| const Group& getGroup(const std::string& groupName, std::size_t timeStep) const; | ||
| bool hasChoke(const std::string& groupName, std::size_t timeStep) const; |
There was a problem hiding this comment.
@plgbrts , you changed to use nodeName in the .cpp file, it is good to have the consistency in the .hpp file also.
| return this->snapshots[timeStep].groups.get(groupName); | ||
| } | ||
|
|
||
| bool Schedule::hasChoke(const std::string& nodeName, std::size_t timeStep) const { |
There was a problem hiding this comment.
When reviewing the downstream PR, I saw there is the following function,
bool Group::as_choke() const {
return this->m_choke_group.has_value();
}Does this function overlap with the new function introduced here?
There was a problem hiding this comment.
Does this function overlap with the new function introduced here?
Kind of. This particular function exists for the "standard network" model (GRUPNET keyword).
There was a problem hiding this comment.
But it looks like updated in the function handleNODEPROP(), which is for extended network.
Potentially we can reuse as_choke() function instead of introducing a new one?
3 participants
No description provided.