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

vkip · 2024-06-05T07:39:06Z

vkip · 2024-06-05T09:18:26Z

jenkins build this opm-common=4094 please

GitPaean

hei, I struggled to understand the roles of m_network_production_rates and node_inflows. It is node_inflows that is used to update the network pressure. There is some overlap between these two.

I left some comments for consideration and discussion.

GitPaean · 2024-06-13T08:15:57Z

opm/simulators/utils/PartiallySupportedFlowKeywords.cpp

@@ -284,7 +284,7 @@ partiallySupported()
         {
            "WEFAC",
            {
-               {3,{true, allow_values<std::string> {"YES"}, "WEFAC(WELNETWK): only the YES option is supported"}}, // EXTENDED_NETWORK_OPT
+               {3,{true, allow_values<std::string> {"YES","NO"}, "WEFAC(WELNETWK): value must be YES or NO"}}, // USE_WEFAC_IN_NETWORK


With this PR, should we move WEFAC and GEFAC out of PartiallySupportedFlowKeywords.cpp file.

We could, and probably should, do that, but I think we give users a better error message by leaving it in there.

By explicitly listing only YES and NO as supported we get, e.g.:

WEFAC: invalid value 'YESss' for item 3 In file: TEST.DATA, line NNN WEFAC(WELNETWK): value must be YES or NO

whereas we get something like the following (and at a later stage) if we remove the keywords from PartiallySupported:

In TEST.DATA line NNN Internal error: Could not convert string 'NOSE' to bool

So what I would actually like is better first-level checking of fully supported keywords, but maybe we can leave it as is until that is in place?

I share your view regarding what is a good error message. At the same time, for the item 3, it is only supposed to have YES or NO two options, then we are fully supporting item (keyword) when we can handle YES and NO, it is a little weird to have it in the file that indicating PartiallySupported.

Not sure whether we should enforce good error message through this strategy. Maybe @bska can comment?

It is not optimal, for sure, but maybe ok for now if I add a comment that it is actually fully supported and kept in there for better validation?

My current view is that we should enforce the good error message in the parser side. Unless we want to have a FullySupportedFlowKeywords.cpp to regulate from the simulator side in an systematic level. It is confusing to have it in the PartiallySupportedFlowKeywords file.

Ok, fair enough, will remove and extend handling in handle?EFAC.

Actually I changed my mind on this one, so added support for 'FullySupportedKeywords' within the current validation framework. With this we can pick up multiple simple parsing errors early and report them to the user, instead of taking them one-by-one in the keyword handlers. Added WEFAC and GEFAC for now, but with the framework in place it should be easy to add more keywords as we see fit.

GitPaean · 2024-06-13T08:28:53Z

opm/simulators/wells/GroupState.cpp

+template<class Scalar>
+bool GroupState<Scalar>::has_network_production_rates(const std::string& gname) const
+{
+    auto group_iter = this->m_network_production_rates.find(gname);


If you want, you can use the following,

return this->m_network_production_rates.count(gname) > 0;

to me it is easier to read and less code. but you do not have to.

And also, I do not think this function is used anywhere. Maybe can consider to remove it.

I don't disagree, but I just copied the style of the existing 'has_production_rates' function. Can update that as well then. I would like to keep the function, I expect it to be required for output purposes (see below).

It is fine and partly personal preference. When we support c++20, we can use the member function contains.

Ok, I'll leave this as is then 👍

GitPaean · 2024-06-14T09:38:25Z

opm/simulators/wells/WellGroupHelpers.cpp

+        std::vector<Scalar> network_rates = rates;
+        if (network.needs_instantaneous_rates(schedule, reportStepIdx)) {
+            for (int phase = 0; phase < np; ++phase) {
+                network_rates[phase] = sumWellPhaseRates(false, group, schedule, wellState, reportStepIdx, phase, /*isInjector*/ false, /*network*/ true);


I can be wrong here. It looks like for the network_production_rates, we only use the values for all the leaf_nodes in its current form.

Maybe we should only need to do differently for the leaf_nodes. It looks like we will deal with other non-leaf nodes in the function computeNetworkPressures based on the rates of the leaf_nodes.

If that sounds correct to you, maybe we can deal with this in the function computeNetworkPressures where the leaf_nodes information are available?

You are right, only the leaf nodes are used in computeNetworkPressures, but this seemed to be the easies way to get those correctly. Also, to be able to report the rates I expect we will need to have all the rates actually used in the network available.

I may have missed something, but to handle this entirely within the computeNetworkPressures it appears that large parts of the sumWellPhaseRates function would need to be duplicated (essentially everything below the recursive call), so I fail to see how that would be simpler (?)

One important thing missing from current m_network_production_rates in the current form is that, if a network node is not in the GRUPTREE, it will not be in the m_network_production_rates. Because network nodes and group nodes can be different.

And also, can we call sumWellPhaseRates through computeNetworkPressures in some way instead of duplicating them. I could not tell why we can not do that by looking at the code.

Ideally, we might need a function called updateNetworkProductionRates(). You do not have to do it unless you are really up to it. We can make a refactoring PR following this PR.

We do not yet support NEFAC so the rates of non-group nodes would always be full rates, but I can see that it probably would be cleaner to also have these stored and co-located with the others. This should be fairly straightforward to change so I can have a go at it.

Regarding the leaf node rates, if I have understood tings correctly this requires global communication after calling sumWellPhaseRates, so I still think it is better to leverage the machinery in GroupState for this. If you insist on doing this within computeNetworkPressures I kindly ask that you provide a working implementation.

You are correct when coming to the parallel communication. I did not think about it and my apologizes. Will respond with more looking into it.

No worries, and thanks! I'll await further input then.

(without considering NEFAC, then basically, NEFAC is 1.0 for the nodes not in the GRUPTREE)

In the current way, m_network_production_rates holds the values for all the group nodes. node_inflows holds a complete version of the rates for all the network nodes.

When all the group nodes are in the network, for the nodes they both contain, they should have the same values. although calculated through different paths and NEFAC is 1.0.

For the situations where some group nodes are not in the network, for example, they have different levels of hierarchy, how to calculate the values (using the efficiency factors) I think is something we can only test to find out, it is not totally clear to me about how things should be done. Likely, m_network_production_rates can have different values from the values in node_inflows, depending on the understanding we use at the end.

In the current code, computeNetworkPressures is going through the network branches, and updateGroupProductionRates is going through the group tree. We do not want to do global communication in the function computeNetworkPressures. Ideally we can borrow the function updateGroupProductionRates to get leaf-nodes (only leaf-nodes), and handle the network-way of production rates in computeNetworkPressures.

Now I agree with you that the current way is a simpler way to get things done. And also, I see that with NEFAC and nodes in the GRUPTREE and not in the network probably introduces re-working in these parts. We can get the results correct first and follow with re-factoring in these parts.

Sorry for the previous misunderstandings.

No problem, I can see it is confusing with the member m_network_production_rates where only leaf rates are used, and will try to make it more clear with proper comments. Glad we can agree to move forward with the current overall approach for now. Thank you.

When all the group nodes are in the network, for the nodes they both contain, they should have the same values. although calculated through different paths and NEFAC is 1.0.

Just for record in case it helps understanding and design or for later usage.

Not totally sure though. With extended network model, we might have different hierarchy layout between nodes, then for the non-leaf nodes, they will have very different values between m_network_production_rates and node_inflows. Luckily, the leaf nodes in the m_network_production_rates should still have the correct/desired value. node_inflows will most likely be the one holding the correct/desired values that represent the network structure.

… item 3)

…ly supported keywords.

GitPaean · 2024-06-27T07:18:57Z

Please give a hint when the update is finished with this PR. And maybe @bska can have a look at the changes related to KeywordValidition and FullySupportedFlowKeywords?

vkip · 2024-06-28T09:33:14Z

Please give a hint when the update is finished with this PR. And maybe @bska can have a look at the changes related to KeywordValidition and FullySupportedFlowKeywords?

Will do, placing this and the companion in opm-common in draft for now.

vkip mentioned this pull request Jun 5, 2024

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 mentioned this pull request Jun 11, 2024

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

Draft

vkip force-pushed the extnet_efficiency branch from ebba0d2 to 1564d06 Compare June 12, 2024 12:38

GitPaean reviewed Jun 14, 2024

View reviewed changes

vkip added 3 commits June 24, 2024 09:26

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

086eb65

… item 3)

Updating item names

6515c63

Adding 'FullySupportedKeywords' to find easy errors early also in ful…

db6e623

…ly supported keywords.

vkip force-pushed the extnet_efficiency branch from 5d6f98e to db6e623 Compare June 24, 2024 07:26

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

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) #5410

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

vkip commented Jun 5, 2024

vkip commented Jun 5, 2024

GitPaean left a comment

GitPaean Jun 13, 2024

vkip Jun 17, 2024 •

edited

Loading

GitPaean Jun 17, 2024

vkip Jun 17, 2024

GitPaean Jun 18, 2024

vkip Jun 18, 2024

vkip Jun 24, 2024

GitPaean Jun 13, 2024

vkip Jun 17, 2024

GitPaean Jun 17, 2024

vkip Jun 17, 2024

GitPaean Jun 14, 2024 •

edited

Loading

vkip Jun 17, 2024

GitPaean Jun 18, 2024

vkip Jun 18, 2024

GitPaean Jun 18, 2024 •

edited

Loading

vkip Jun 18, 2024

GitPaean Jun 18, 2024

vkip Jun 18, 2024

GitPaean Jun 18, 2024

GitPaean commented Jun 27, 2024

vkip commented Jun 28, 2024

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

Are you sure you want to change the base?

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

Conversation

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

vkip Jun 17, 2024 • edited Loading

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

GitPaean Jun 14, 2024 • edited Loading

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 Jun 18, 2024 • edited Loading

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

GitPaean commented Jun 27, 2024

vkip commented Jun 28, 2024

vkip Jun 17, 2024 •

edited

Loading

GitPaean Jun 14, 2024 •

edited

Loading

GitPaean Jun 18, 2024 •

edited

Loading