Additional Functionality needed for PINCH Option(4) ALL support #4181

blattms · 2024-09-04T12:22:10Z

We will need to be able to

Calculate vertical transmissibilities during grid setup, which needs cell and face centers and face normal. Hence there is a function to get those.
We need an easy way to add multiple NNCs at once that will also hold transmissibilities. Those NNCs will be used to overwrite transmissibilities over pinched out cells in this case.

There are upcoming PRs in opm-grid and opm-simulators, but they still need some. I'll make this WIP because of that as changes migth be needed.

blattms · 2024-09-04T12:22:32Z

jenkins build this please

blattms · 2024-09-04T12:31:28Z

One question here is also whether to set EclipseGrid::m_multzMode to TOP unconditionally if EclipseGrid::m_pinchoutMode is ALL.

blattms · 2024-09-04T20:06:23Z

jenkins build this opm-grid=756 opm-simulators=5577 please

opm/input/eclipse/EclipseState/Grid/NNC.hpp

blattms · 2024-09-05T10:36:55Z

jenkins build this opm-grid=756 opm-simulators=5577 please

atgeirr

Looks good, should be merged after addressing issues. One potential (not new!) bug, otherwise mostly suggestions for small improvements.

opm/input/eclipse/EclipseState/Grid/FieldPropsManager.cpp

opm/input/eclipse/EclipseState/Grid/FieldPropsManager.hpp

atgeirr · 2024-09-12T12:47:34Z

opm/input/eclipse/EclipseState/Grid/EclipseGrid.hpp

@@ -160,6 +160,9 @@ namespace Opm {
        /// Will return a vector a length num_active; where the value
        /// of each element is the corresponding global index.
        const std::vector<int>& getActiveMap() const;
+        /// \brief get cell center, and center and normal of bottom face
+        std::tuple<std::array<double, 3>,std::array<double, 3>,std::array<double, 3>>


This is fine, but if/when we merge this module with opm-grid, then Dune::FieldVector would be available. Do you think that would be a good idea to user throughout here?

It might reduce code duplication and facilitate code reuse, indeed. ... once the modules are merged.

opm/input/eclipse/EclipseState/Grid/EclipseGrid.cpp

atgeirr · 2024-09-12T13:36:34Z

opm/input/eclipse/EclipseState/Grid/EclipseGrid.cpp

+                                        *(Y.begin() + 6),
+                                        *(Z.begin() + 6)};
+
+        for(const auto cornerIndex: bottomIndices)


I suggest instead doing the cross product of the diagonals, that is the same order of accuracy, and less complex to write and read.

I know that the code is complex. It basically mirrors the way it is done in opm-grid (which does the same but due to lack of docs is even harder to understand). I had the same reaction when I saw that code as you now.

But (after contemplating about the original code for some time) I think that the complexity is there for a reason. The problem is that we are talking about corner point grids. Where the corners of the top and bottom face do not need to span a plan. For a plane there is no difference,
But those points can e.g. lie rather awkwardly on the columns, such that the face is actually kind of curved and flat. Then doing it this way (with the additional center of mass) is a better approximation to the curved "plane", I think...

opm/input/eclipse/EclipseState/Grid/NNC.hpp

atgeirr · 2024-09-12T13:43:56Z

opm/input/eclipse/EclipseState/Grid/NNC.cpp

+                std::swap(item.cell1, item.cell2);
+        });
+        std::sort(m_input.begin() + old_size, m_input.end());
+        std::inplace_merge(m_input.begin(), m_input.begin() + old_size, m_input.end());


This requires that both partial ranges are sorted, and for the incoming data that is ensured here in this function. Could this be changed to combine two NNC objects instead, or would that be inefficient for some reason? Then both ranges would already be sorted, since addNNC() already ensures this.

Probably. I will see what I can do.
Not even sure anymore, whether I will need this.

Actually, there is nothing to be done here. as the m_input should always be sorted, see load_input and the likes.

They are both sorted, see load_input and the likes.

If so, then this function can be reduced to just:

const auto old_size = m_input.size(); m_input.insert(m_input.end(), data.begin(), data.end()); std::inplace_merge(m_input.begin(), m_input.begin() + old_size, m_input.end());

(If my reading of load_input() is correct.)

I should not have said both.

I do not want to rely on anything here for the function parameter. Hence I need to make sure that the indices are correct and sort that one. That is what I am doing.

blattms

thanks a lot for the review. I will try to address most of those. First I need to fix some still lingering bugs (materializing in the real wold) in the approach, though.

blattms · 2024-09-17T14:23:48Z

opm/input/eclipse/EclipseState/Grid/EclipseGrid.cpp

+                                        *(Y.begin() + 6),
+                                        *(Z.begin() + 6)};
+
+        for(const auto cornerIndex: bottomIndices)


I know that the code is complex. It basically mirrors the way it is done in opm-grid (which does the same but due to lack of docs is even harder to understand). I had the same reaction when I saw that code as you now.

But (after contemplating about the original code for some time) I think that the complexity is there for a reason. The problem is that we are talking about corner point grids. Where the corners of the top and bottom face do not need to span a plan. For a plane there is no difference,
But those points can e.g. lie rather awkwardly on the columns, such that the face is actually kind of curved and flat. Then doing it this way (with the additional center of mass) is a better approximation to the curved "plane", I think...

blattms · 2024-09-17T14:24:56Z

opm/input/eclipse/EclipseState/Grid/EclipseGrid.hpp

@@ -160,6 +160,9 @@ namespace Opm {
        /// Will return a vector a length num_active; where the value
        /// of each element is the corresponding global index.
        const std::vector<int>& getActiveMap() const;
+        /// \brief get cell center, and center and normal of bottom face
+        std::tuple<std::array<double, 3>,std::array<double, 3>,std::array<double, 3>>


It might reduce code duplication and facilitate code reuse, indeed. ... once the modules are merged.

blattms · 2024-09-17T14:27:51Z

opm/input/eclipse/EclipseState/Grid/NNC.cpp

+                std::swap(item.cell1, item.cell2);
+        });
+        std::sort(m_input.begin() + old_size, m_input.end());
+        std::inplace_merge(m_input.begin(), m_input.begin() + old_size, m_input.end());


Probably. I will see what I can do.
Not even sure anymore, whether I will need this.

opm/input/eclipse/EclipseState/Grid/NNC.hpp

blattms · 2024-09-23T13:25:58Z

jenkins build this opm-grid=756 opm-simulators=5577 please

blattms · 2024-09-24T15:46:15Z

jenkins build this opm-grid=756 opm-simulators=5577 please

blattms · 2024-09-24T20:33:16Z

jenkins build this opm-grid=756 opm-simulators=5577 please

bska · 2024-09-26T08:22:09Z

I notice that there's a merge conflict here, but that the PR otherwise is marked ready for review. What's the status of this overall feature–i.e., this PR and its downstream companion PRs? Is everything ready for review?

blattms · 2024-09-27T09:39:02Z

Shoot. Somehow I forgot pushing yesterday.
This is now working for the test cases and the model that I have.

blattms · 2024-09-27T09:39:31Z

jenkins build this opm-grid=756 opm-simulators=5577 please

blattms · 2024-09-30T09:39:30Z

I will rebase this to make it work for users again.

Just note, that I am factoring out another PR (for OPERATER), but some tests are still giving bad results-

This is needed to calculate Z transmissibilities for pinched out cells.

This will be needed when computing transmissibilties for pinched cells. Then we will need to apply them only for a subset of cells.

This is needed for PINCH optio 4 ALL.

These are the keywords that are needed to calculate transmissibilties for the deactivated cells for the case that PINCH(4) is ALL: PERM*, TRANZ* (for the transmissibility multiplier). After we process PINCH we make those local again. To mark these keywords we use keyword_info::local_in_schedule

This cleaner and does not increase the storage size for the normal NNCs. It will also be needed for correct multiplier application.

blattms · 2024-10-01T07:36:37Z

jenkins build this opm-grid=756 opm-simulators=5577 please

blattms · 2024-10-01T07:50:11Z

Unfortunately my answers are not seen here in threaded mode. I gues you have to go to the "Files changed" tab to see the context. Seems like there currently is nothing to here.

blattms · 2024-10-01T07:50:21Z

jenkins build this opm-grid=756 opm-simulators=5577 please

bska · 2024-10-01T08:24:58Z

jenkins build this opm-grid=756 opm-simulators=5577 please

blattms · 2024-10-01T08:56:07Z

Why the heck should this change alugrid results?

Branches were out of sync. Retrying...

blattms · 2024-10-01T08:56:17Z

jenkins build this opm-grid=756 opm-simulators=5577 please

blattms · 2024-10-01T09:16:38Z

Seems like my push in opm-simulators was previously not successful. Let's try again

blattms · 2024-10-01T09:17:14Z

jenkins build this opm-grid=756 opm-simulators=5577 please

atgeirr

Having reread the changed parts, I think that aside from some very minor issues, this is ready to be merged. Will be looking at the PRs in the other repos next.

atgeirr · 2024-10-01T14:31:17Z

opm/input/eclipse/EclipseState/Grid/Keywords.hpp

    bool global = false;
+    /// \brief Supply global storage but remove it once the SCHEDULE is executed
+    ///
+    /// Needed to get rid off global storage for keywords needed for PINCH


Typo: "rid of".

atgeirr · 2024-10-01T14:32:21Z

opm/input/eclipse/EclipseState/Grid/Keywords.hpp

        return *this;
    }

+    keyword_info<T>& global_kw_until_edit() {
+        this->global = this->local_in_schedule = true;


While correct code, it is easy to misread in a hurry. I would very much prefer a single assignment per line.

opm/input/eclipse/EclipseState/Grid/FieldPropsManager.hpp

blattms · 2024-10-01T18:53:58Z

jenkins build this opm-grid=756 opm-simulators=5577 please

atgeirr · 2024-10-02T07:44:48Z

jenkins build this please

blattms · 2024-10-03T10:50:22Z

Can be merged alone but triggering jenkins for the whole chain anyway.

blattms · 2024-10-03T10:50:28Z

jenkins build this opm-grid=756 opm-simulators=5577 please

blattms · 2024-10-04T13:47:47Z

And another time. To test typo fixes in opm-grid...

blattms · 2024-10-04T13:47:50Z

jenkins build this opm-grid=756 opm-simulators=5577 please

blattms changed the title ~~Additional Functionality needed support PINCH Option(4) ALL~~ Additional Functionality needed for PINCH Option(4) ALL support Sep 4, 2024

This was referenced Sep 4, 2024

Precalculate TRANZ because of PINCH option 4 ALL and include as NNC. OPM/opm-grid#756

Merged

Correctly treat transmissibilities for PINCH option(4) ALL. OPM/opm-simulators#5577

Merged

blattms commented Sep 4, 2024

View reviewed changes

opm/input/eclipse/EclipseState/Grid/NNC.hpp Outdated Show resolved Hide resolved

blattms marked this pull request as ready for review September 5, 2024 11:39

atgeirr reviewed Sep 12, 2024

View reviewed changes

blattms commented Sep 17, 2024

View reviewed changes

blattms force-pushed the feature/get-centers-egrid branch from e87c5a5 to c9bea68 Compare September 20, 2024 16:17

blattms marked this pull request as draft September 20, 2024 16:21

blattms force-pushed the feature/get-centers-egrid branch 2 times, most recently from e035942 to 97afe21 Compare September 23, 2024 11:28

blattms marked this pull request as ready for review September 24, 2024 15:42

blattms force-pushed the feature/get-centers-egrid branch from 4006afc to 6240728 Compare September 24, 2024 15:45

blattms force-pushed the feature/get-centers-egrid branch from 6240728 to 4d8ba69 Compare September 27, 2024 09:36

blattms force-pushed the feature/get-centers-egrid branch from 4d8ba69 to 990b6be Compare September 27, 2024 14:57

blattms mentioned this pull request Sep 27, 2024

Support COPY for FieldData with global storage. #4236

Merged

blattms force-pushed the feature/get-centers-egrid branch from 990b6be to 6de2cdb Compare September 27, 2024 15:41

blattms mentioned this pull request Sep 27, 2024

Support OPERATE for FieldData with global storage. #4237

Merged

blattms force-pushed the feature/get-centers-egrid branch from 6de2cdb to 5fbe4f9 Compare September 30, 2024 09:49

blattms mentioned this pull request Sep 30, 2024

Allow region operator on global 3D fields (poor man's version) #4238

Merged

blattms force-pushed the feature/get-centers-egrid branch from 5fbe4f9 to e2ef76a Compare September 30, 2024 11:36

blattms added 7 commits October 1, 2024 09:34

Get cell center and and center and normal of bottom face at once.

60983f4

This is needed to calculate Z transmissibilities for pinched out cells.

Refactor how we apply transmissibility modifcations in FieldProps.

52261b8

This will be needed when computing transmissibilties for pinched cells. Then we will need to apply them only for a subset of cells.

Allow applying TRANS modifiers only to index ranges.

d91c333

This is needed for PINCH optio 4 ALL.

Support merging a list of additional NNCs to existing.

199c442

Improve documentation for PINCH members of EclipseGrid

eecfd76

[refactor] Store NNCs from PINCH separately.

10ad859

This cleaner and does not increase the storage size for the normal NNCs. It will also be needed for correct multiplier application.

blattms force-pushed the feature/get-centers-egrid branch from 30f6ded to 10ad859 Compare October 1, 2024 07:35

atgeirr reviewed Oct 1, 2024

View reviewed changes

Address reviewer's comments.

adf6645

atgeirr approved these changes Oct 4, 2024

View reviewed changes

blattms merged commit 7459cdf into OPM:master Oct 4, 2024
1 check passed

blattms deleted the feature/get-centers-egrid branch October 4, 2024 19:02

Additional Functionality needed for PINCH Option(4) ALL support #4181

Additional Functionality needed for PINCH Option(4) ALL support #4181

Conversation

blattms commented Sep 4, 2024

blattms commented Sep 4, 2024

blattms commented Sep 4, 2024

blattms commented Sep 4, 2024

blattms commented Sep 5, 2024

atgeirr 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

blattms Sep 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

blattms left a comment

Choose a reason for hiding this comment

blattms Sep 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

blattms commented Sep 23, 2024

blattms commented Sep 24, 2024

blattms commented Sep 24, 2024

bska commented Sep 26, 2024

blattms commented Sep 27, 2024

blattms commented Sep 27, 2024

blattms commented Sep 30, 2024

blattms commented Oct 1, 2024

blattms commented Oct 1, 2024

blattms commented Oct 1, 2024

bska commented Oct 1, 2024

blattms commented Oct 1, 2024

blattms commented Oct 1, 2024

blattms commented Oct 1, 2024

blattms commented Oct 1, 2024

atgeirr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blattms commented Oct 1, 2024

atgeirr commented Oct 2, 2024

blattms commented Oct 3, 2024

blattms commented Oct 3, 2024

blattms commented Oct 4, 2024

blattms commented Oct 4, 2024

blattms Sep 17, 2024 •

edited

Loading

blattms Sep 17, 2024 •

edited

Loading