Support instantaneous flow rates in extended network (WEFAC and GEFAC item 3) #4094

vkip · 2024-06-05T07:33:41Z

No restart support here - will come in a separate PR.

vkip · 2024-06-05T07:42:51Z

jenkins build this opm-simulators=5410 please

vkip · 2024-06-05T08:00:41Z

jenkins build this opm-simulators=5410 please

GitPaean

It is not a lot of code, but I do feel it can be tricky. I left some comments for your consideration.

opm/input/eclipse/Schedule/Network/ExtNetwork.hpp

opm/input/eclipse/Schedule/Network/ExtNetwork.cpp

opm/input/eclipse/Schedule/Well/WellPropertiesKeywordHandlers.cpp

GitPaean · 2024-06-11T12:18:43Z

opm/input/eclipse/Schedule/Network/ExtNetwork.cpp

@@ -205,6 +212,36 @@ void ExtNetwork::update_node(Node node)
    this->m_nodes.insert_or_assign(name, std::move(node) );
 }

+bool ExtNetwork::needs_instantaneous_rates(const Opm::Schedule& schedule, const int report_step) const {


Based on the usage of this function in OPM/opm-simulators#5410, it basically indicates whether we are handling network flow with extended network with "NO" in the item 3 in WEFAC or GEFAC.

Or more simply, we can use true for extended network and use false for standard network and group calculation. If we can introduce a flag to mark whether the network is extended network, we do not need this function anymore. And use_efficiency_in_ext_network will take care whether we will use instantaneous rate.

What do you think? I can understand this part wrong. Please let me know.

This function can certainly be skipped, but the intention is to save some computation in opm-simulators in the quite common situation with default value (YES) for all instances of WEFAC/GEFAC with wells/groups in the network.

One complication I see is action handling (i.e., if item3 is updated through actions), will need to ensure the check is performed again in this case. Can have a quick look, or we just drop this for now.

Removed this now.

opm/input/eclipse/Schedule/Group/Group.cpp

GitPaean · 2024-06-13T07:46:01Z

jenkins build this opm-simulators=5410 please

vkip · 2025-01-17T15:00:53Z

jenkins build this opm-simulators=5410 please

vkip · 2025-01-20T12:29:44Z

jenkins build this opm-simulators=5410 please

GitPaean · 2025-02-11T21:39:39Z

jenkins build this opm-simulators=5410 please

GitPaean

It looks generally good to me. I only have some minor comments for you to address.

GitPaean · 2025-02-12T11:56:05Z

opm/input/eclipse/Schedule/Network/Node.hpp

    bool add_gas_lift_gas() const;
    const std::optional<std::string>& target_group() const;

    void terminal_pressure(double pressure);
    void add_gas_lift_gas(bool add_gas);
    void as_choke(const std::string& target_group);
+    void set_efficiency(const double& efficiency);


the reference & here is not necessary.

GitPaean · 2025-02-12T12:32:26Z

opm/input/eclipse/Schedule/Network/ExtNetwork.cpp

+                children.push(branch.downtree_node());
+            }
+        }
+        assert(children.empty());


with while (!children.empty()), the assertion here is not necessary.

GitPaean · 2025-02-12T12:35:24Z

opm/input/eclipse/Schedule/Network/ExtNetwork.cpp

+        std::stack<std::string> children;
+        children.push(root.get().name());
+        while (!children.empty()) {
+            const auto node = children.top();


nittpicking, const auto& node can be used here.

And leaf_nodes.emplace(node) can be used in the code below instead of insert().

GitPaean · 2025-02-12T19:56:15Z

opm/input/eclipse/Schedule/Well/Well.cpp

-double Well::getEfficiencyFactor() const
-{
+double Well::getEfficiencyFactor(bool network) const {
+    if (network && (!this->use_efficiency_in_network)) {


(!this->use_efficiency_in_network), the bracket is not necessary with a ! operator when coming to improving the readability.

GitPaean · 2025-02-12T19:59:20Z

opm/input/eclipse/Schedule/Well/Well.cpp

    return this->efficiency_factor;
 }

-double Well::getSolventFraction() const
-{
+bool Well::useEfficiencyInNetwork() const {


this function does not look like used.

GitPaean · 2025-02-12T20:04:00Z

opm/input/eclipse/Schedule/Well/Well.hpp

@@ -517,7 +518,7 @@ class Well {
    bool updateWellGuideRate(bool available, double guide_rate, GuideRateTarget guide_phase, double scale_factor);
    bool updateWellGuideRate(double guide_rate);
    bool updateAvailableForGroupControl(bool available);
-    bool updateEfficiencyFactor(double efficiency_factor);
+    bool updateEfficiencyFactor(double efficiency_factor, bool se_efficiency_in_network = true);


The default value for bool se_efficiency_in_network = true is not very useful, since the only usage of the function is for parsing keyword WEFAC, the default behavoir is specified through the definition of the keyword WEFAC.

Sure, updated a test instead.

GitPaean · 2025-02-12T20:11:17Z

opm/input/eclipse/Schedule/Network/ExtNetwork.cpp

@@ -235,5 +239,29 @@ std::vector<std::string> ExtNetwork::node_names() const
 {
    return this->insert_indexed_node_names;
 }
+
+std::set<std::string> ExtNetwork::leaf_nodes() const


The function looks correct to me. From the usage in the opm-simulators, it looks like it will be called many times. Ideally, it will be nice if we can save the results somehow to avoid repeating the process.

This can be done in a following up PR if it turns out to be costly and needs to be optimized.

GitPaean · 2025-02-12T20:23:52Z

opm/input/eclipse/Schedule/Well/Well.cpp

@@ -2038,6 +2052,7 @@ bool Well::cmp_structure(const Well& other) const
        && (this->udq_undefined == other.udq_undefined)
        && (this->getPreferredPhase() == other.getPreferredPhase()) // wellType()
        && (this->getEfficiencyFactor() == other.getEfficiencyFactor())


It does not matter for the current usage since the argument is false by default, but it might be better to compare the value
this->efficiency_factor == other.efficiency_factor now, since the function getEfficiencyFactor() uses use_efficiency_in_network and it is a little more complex function now.

It does not matter for the current usage since the argument is false by default, but it might be better to compare the value this->efficiency_factor == other.efficiency_factor now, since the function getEfficiencyFactor() uses use_efficiency_in_network and it is a little more complex function now.

Arguably, two wells aren't equal if their use_efficiency_in_network differ so I think it actually makes sense to continue to use getEfficiencyFactor() for this criterion.

Sure.

In the current form of the function double getEfficiencyFactor(bool network), it is fine and it is equivalent to compare with the efficiency_factor. But I do feel that it is safer (and slightly faster) to compare with the member variable directly.

In a more general setting, for example, the function double getEfficiencyFactor(bool network) evolves somehow it might not reflect the value of efficiency_factor, then a bug can be introduced. Or we might need to compare both getEfficiencyFactor(true) and getEfficiencyFactor(false).

As said, I think it does not make much difference for the current code, but I do think compare with the member variable itself is safer generally.

… item 3)

vkip · 2025-02-14T09:48:54Z

It looks generally good to me. I only have some minor comments for you to address.

Thank you for the review. I have now tried to update accordingly.

vkip · 2025-02-14T10:38:40Z

jenkins build this opm-simulators=5410 please

GitPaean · 2025-02-14T13:36:07Z

Thanks for the update. It looks good to me and I am getting it in now.

This was referenced Jun 5, 2024

Support instantaneous flow rates in extended network (WEFAC and GEFAC item 3) OPM/opm-simulators#5410

Merged

Test case for supporting instantaneous rates in extended network OPM/opm-tests#1175

Merged

vkip requested review from bska and GitPaean June 5, 2024 07:43

GitPaean requested changes Jun 11, 2024

View reviewed changes

vkip force-pushed the extnet_efficiency branch from 1dfbddd to cae2f0c Compare June 12, 2024 12:36

vkip marked this pull request as draft June 28, 2024 09:33

vkip force-pushed the extnet_efficiency branch from a5c4c31 to e1f9c8d Compare January 17, 2025 13:32

vkip marked this pull request as ready for review January 17, 2025 15:27

vkip force-pushed the extnet_efficiency branch from e1f9c8d to 9102c16 Compare January 20, 2025 12:25

GitPaean requested changes Feb 12, 2025

View reviewed changes

GitPaean reviewed Feb 12, 2025

View reviewed changes

vkip added 3 commits February 14, 2025 10:47

Support instantaneous flow rates in extended network (WEFAC and GEFAC…

f28a9fb

… item 3)

Implement NEFAC + clean up implementation

470605e

Minor fixed after review

20734b9

vkip force-pushed the extnet_efficiency branch from 9102c16 to 20734b9 Compare February 14, 2025 09:48

GitPaean merged commit 63c3159 into OPM:master Feb 14, 2025
1 check passed

vkip deleted the extnet_efficiency branch February 17, 2025 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support instantaneous flow rates in extended network (WEFAC and GEFAC item 3) #4094

Support instantaneous flow rates in extended network (WEFAC and GEFAC item 3) #4094

vkip commented Jun 5, 2024

vkip commented Jun 5, 2024

vkip commented Jun 5, 2024

GitPaean left a comment

GitPaean Jun 11, 2024

vkip Jun 12, 2024

vkip Jan 20, 2025

GitPaean commented Jun 13, 2024

vkip commented Jan 17, 2025

vkip commented Jan 20, 2025

GitPaean commented Feb 11, 2025

GitPaean left a comment

GitPaean Feb 12, 2025

GitPaean Feb 12, 2025

GitPaean Feb 12, 2025

GitPaean Feb 12, 2025

GitPaean Feb 12, 2025

GitPaean Feb 12, 2025

vkip Feb 14, 2025

GitPaean Feb 12, 2025

GitPaean Feb 12, 2025

bska Feb 13, 2025

GitPaean Feb 13, 2025

vkip commented Feb 14, 2025

vkip commented Feb 14, 2025

GitPaean commented Feb 14, 2025

Support instantaneous flow rates in extended network (WEFAC and GEFAC item 3) #4094

Support instantaneous flow rates in extended network (WEFAC and GEFAC item 3) #4094

Conversation

vkip commented Jun 5, 2024

vkip commented Jun 5, 2024

vkip commented Jun 5, 2024

GitPaean left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitPaean commented Jun 13, 2024

vkip commented Jan 17, 2025

vkip commented Jan 20, 2025

GitPaean commented Feb 11, 2025

GitPaean left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkip commented Feb 14, 2025

vkip commented Feb 14, 2025

GitPaean commented Feb 14, 2025