using pinched grid for numerical aquifers. by GitPaean · Pull Request #517 · OPM/opm-grid

GitPaean · 2021-03-25T22:48:31Z

No description provided.

GitPaean · 2021-03-26T23:03:26Z

serial results look fine while parallel results are wrong.

opm/grid/cpgrid/processEclipseFormat.cpp

GitPaean · 2021-04-07T18:50:21Z

the parallel results looks fine now. fixing all the tests now.

GitPaean · 2021-04-07T22:16:41Z

In this PR, we process the numerical aquifers connection information in the middle of the grid processing, which does not look natural, but I do not think it is an easy task to refactor to do the PINCH, MINPV in the parser. I am marking this PR to be ready for review.

blattms · 2021-04-08T08:35:24Z

opm/grid/cpgrid/CpGridData.hpp

    /// \param remove_ij_boundary if true, will remove (i, j) boundaries. Used internally.
-    void processEclipseFormat(const grdecl& input_data, const std::array<std::set<std::pair<int, int>>, 2>& nnc, double z_tolerance, bool remove_ij_boundary, bool turn_normals = false,
-                              const std::unordered_map<size_t, double>& aquifer_cell_volumes = std::unordered_map<size_t, double>());
+    void processEclipseFormat(const grdecl& input_data, Opm::EclipseState& ecl_state, const Opm::Deck& deck,


Your really need to document this properly, as the change seems a bit dangerous. We seem to call this function with an empty ecl_state, which is fine as long as there are no numerical aquifers.

The danger is that in the future fellow developer might assume that the ecl_state is always valid and use it for other things. In that case they will have a hard time debugging.

This looks very dangerous to me - the empty EclipseState is purely an artifact of the serialization - it is deeply invalid and should not be used for anything!

I will add the document here. If we do not use EclipseState here, we will need the EclipseGrid, FieldPropsManager, AquiferConfig and inputNNC of the EclipseState. I really do not want to add so many arguments and passing them around. One of the big difficulties when developing this part is that we have five different functions with the same name processEclipseFormat, with different arguments, and doing different but related things, and call one other. Maybe it is good idea to rename the functions to get a little bit more descriptive (maybe I should have done it before developing this PR). Anyway, it is not related to this PR directly.

We seem to call this function with an empty ecl_state,

I think you mean the place that uses empty ecl_state to fix the interface.

The empty ecl_state happens in the following code in CpGrid.cpp to fix the interfaces.

void CpGrid::processEclipseFormat(const grdecl& input_data, double z_tolerance, bool remove_ij_boundary, bool turn_normals) { // TODO: just fix the interface, not sure it will be problematic Opm::EclipseState ecl_state; Opm::Deck deck; using NNCMap = std::set<std::pair<int, int>>; using NNCMaps = std::array<NNCMap, 2>; NNCMaps nnc; current_view_data_->processEclipseFormat(input_data, ecl_state, deck, nnc, z_tolerance, remove_ij_boundary, turn_normals);

Do you think to use pointers here better than using the references? Then we can just pass in nullptr for the above situation. Please comment.

Yes I think using a pointer or optional is better here - as I said - things will go fast to pieces if you use a default constructed EclipseState

Okay. I will use pointers here. I think optional to reference is not there yet, right? Correct me if I am wrong here.

Wouldn't std::optional make a copy of EclipseState? That might be expensive...
You would need to use a std::optional with a std::reference_wrapper inside to prevent the copy. Not sure whether that really works.

You would need to use a std::optional with a std::reference_wrapper inside to prevent the copy. Not sure whether that really works.

That works - I have tried it in another context; but maybe slightly over engineered? I would go with a pointer, but this is not my turf.

This change really backfired in the downstream modules. in opm-upscaling module, it only looks at the eclipse_grid. With the same grid deck, you can generate a EclipseGrid and also possible for a EclipseState with an EclipseGrid. But these two grid are different, because since there is no pore volumes, all the grid cells will be marked inactive.

GitPaean · 2021-04-09T22:10:56Z

opm/grid/cpgrid/CpGrid.cpp

+        using NNCMap = std::set<std::pair<int, int>>;
+        using NNCMaps = std::array<NNCMap, 2>;
+        NNCMaps nnc;
+        current_view_data_->processEclipseFormat(g, nullptr, nullptr, nnc, 0.0, false, false);


not sure why nnc can not use {} as the original code, will try more.

not sure why nnc can not use {} as the original code, will try more.

Just a quick follow-up to this in case it's not already clear. The function takes an NNCMaps& rather than a const NNCMaps& so you can't pass a temporary.

GitPaean · 2021-04-10T22:08:01Z

opm/grid/CpGrid.hpp

-                                  const Opm::NNC& = Opm::NNC(),
-                                  const std::unordered_map<size_t, double>& aquifer_cell_volumes = std::unordered_map<size_t, double>());
+        // TODO: add description of the arguments
+        void processEclipseFormat(Opm::EclipseState* ecl_state,


One thing unfortunate due to the change here is that, originally the main input is EclipseGrid, but now the main input EclipseGrid becomes something hidden in the EclipseState.

Maybe we still can keep EclipseGrid as the main input, but also has EclipseState there for extra information. But we can not guarantee the EclipseGrid is the grid from EclipseState.

I will recovered to be the main argument is the EclipseGrid. EclipseState will be secondary argument to provide other information. When there is only EclipseState provided, we use the EclipseGrid belonging to EclipseState.

GitPaean · 2021-04-11T19:37:52Z

opm/grid/cpgrid/processEclipseFormat.cpp

+                }
+                const auto& ecl_grid = ecl_state->getInputGrid();
+                const auto& fp = ecl_state->fieldProps();
+                aquifer.mutableNumericalAquifers().addAquiferConnections(*deck, ecl_grid, new_actnum);


@joakim-hove , it is here the mutableNumericalAquifers() will be modified.

for EclipseState and Deck.

GitPaean · 2021-04-14T11:18:47Z

rebasing turned out to be a little tedious. So merging is used here to fix the conflicts.

GitPaean · 2021-04-16T07:56:21Z

This is the major parts related to this effort to get this piece of numerical aquifers in. Besides some code related to numerical aquifers directly, the main part is interface changes related to functions processEclipseFormat. Please review and suggest.

The jenkins was triggered from upsctream PR OPM/opm-common#2389 and is green now.

bska

Looks good to me. I just had a couple of minor questions. Once those are addressed, I'm prepared to merge this along with its upstream OPM/opm-common#2389.

bska · 2021-04-16T10:54:16Z

opm/grid/cpgrid/CpGrid.cpp

+        using NNCMap = std::set<std::pair<int, int>>;
+        using NNCMaps = std::array<NNCMap, 2>;
+        NNCMaps nnc;
+        current_view_data_->processEclipseFormat(g, nullptr, nullptr, nnc, 0.0, false, false);


not sure why nnc can not use {} as the original code, will try more.

Just a quick follow-up to this in case it's not already clear. The function takes an NNCMaps& rather than a const NNCMaps& so you can't pass a temporary.

bska · 2021-04-16T12:53:33Z

opm/grid/cpgrid/CpGrid.cpp

+        NNCMaps nnc;
+        current_view_data_->processEclipseFormat(input_data, nullptr, nullptr, nnc, z_tolerance, remove_ij_boundary, turn_normals);


Why is it appropriate to just drop the nnc information here?

Not totally sure that I understand your question correctly here.

For this situation, nnc will always be empty. The only reason it can be modified within this function call is numerical aquifers, but we pass in nullptr for EclipseState*, so no numerical aquifers. nnc created here is purely for interface. It does not look pretty.

Suggestion is very welcomed here.

bska · 2021-04-16T14:02:04Z

opm/grid/cpgrid/processEclipseFormat.cpp

-    void CpGridData::processEclipseFormat(const grdecl& input_data, const NNCMaps& nnc, double z_tolerance, bool remove_ij_boundary, bool turn_normals,
-                                          const std::unordered_map<size_t, double>& aquifer_cell_volumes)
+    void CpGridData::processEclipseFormat(const grdecl& input_data, Opm::EclipseState* ecl_state, const Opm::Deck* deck,
+                                          NNCMaps& nnc, double z_tolerance, bool remove_ij_boundary, bool turn_normals)


I didn't notice this before. Strictly speaking, this particular function is not protected by HAVE_ECL_INPUT so it was originally supposed to be callable without types from opm-common. I'd be happy to remove that restriction, but maybe we should discuss this topic a little more before we merge the PR.

Sure. I am not that familiar with grid related stuff.

In my own opinion, the grid processing (getting the final ACTNUM, we should not drop any information before determining the final ACTNUM, so a cell is free to change from inactive to active.) should be done in the early stage of the parsing, so all the numerical aquifers stuff can stay in opm-common.

The current way is intruding because we do the processing (can remove more cells) after parsing, while parsing needs processed grid and parsing information also intervene the grid processing.

…d_numerical_aquifer

blattms · 2021-04-19T06:46:12Z

Thanks for adding documentation. My main point for requesting it, was that developers can see which are optional or which do only have to have a limited state in certain circumstances. Seems like that is still not clear from the text. Or maybe this did change?

I would like to get the following info from the documentation:

If and when I am allowed to pass nullpointers
If and when I allowed to pass incomplete objects (e.g. EclState not reflecting the whole model)

Or has that changed meanwhile.
Would be cool if that was be easy to find. Otherwise I concur with @bska's opinion (not even sure whether I have the required knowledge to do so).

blattms · 2021-04-19T06:47:03Z

I am approving now as this could be done in a separate PR (which often means it will slip).

blattms

Apart from doc improvements this looks fine.

GitPaean · 2021-04-19T07:20:44Z

If and when I am allowed to pass nullpointers

Yes. Null pointers are allowed, which makes it okay to only have the grid passed in. When you do not have ElcipseState or Deck available, you can pass in null pointers. This is referring to the function void CpGridData::processEclipseFormat in processEclipseFormat.cpp.

If and when I allowed to pass incomplete objects (e.g. EclState not reflecting the whole model)

This is hard to tell. For example, if a deck only have the GRID description without providing PORO or PORV information, all the grid cells will be marked inactive. Then it will probably not work very well. But I am not sure it is a problem of the EclipseState or here.

joakim-hove

This PR uses the raw Deck datastructure - outside of opm-common, we must find a way to avoid that. I'm trying to come up with a suggestion.

joakim-hove · 2021-04-19T09:06:50Z

This series of PR's brings in a completely new thing - namely that pinched out cells are activated by the Aquifer code. This is complex code - breaking with an assumption (that cells can only be deactivated) which has been correct for a long time. This should in my opinion not be merged without proper testing - preferably both unit testing and integration test.

joakim-hove · 2021-04-19T09:08:16Z

opm/grid/cpgrid/processEclipseFormat.cpp

+                const size_t global_nc = input_data.dims[0] * input_data.dims[1] * input_data.dims[2];
+                std::vector<int> new_actnum(global_nc, 0);
+                for (int i = 0; i < output.number_of_cells; ++i) {
+                    new_actnum[output.local_cell_index[i]] = 1;


That this line of code actually "turns on" a cell is so special it deserves a comment.

Okay. This is not the line turning on a aquifer cell. This line just generates the actnum based on output.

Here is the place that forcefully keeps aquifer cells active.

opm-grid/opm/grid/cpgpreprocess/preprocess.c

Line 354 in a28f0bd

/* we keep aquifer cells active always even the cells have zero thickness or volume */

joakim-hove · 2021-04-19T09:18:11Z

This PR uses the raw Deck datastructure - outside of opm-common, we must find a way to avoid that. I'm trying to come up with a suggestion.

My only suggestion is to have a temporary vector in the numerical aquiferclass with all the connections coming from the AQUCON keyword, and then a method e.g. NumericalAquifer::post_process_connections( new_actnum );

…d_numerical_aquifer

GitPaean · 2021-04-20T07:35:22Z

My only suggestion is to have a temporary vector in the numerical aquiferclass with all the connections coming from the AQUCON keyword, and then a method e.g. NumericalAquifer::post_process_connections( new_actnum );

Done, waiting for Jenkins to pass. At the same time, there is one function processEclipseFormat using const Deck& as input argument already in CpGridData.hpp.

joakim-hove · 2021-04-20T14:01:23Z

At the same time, there is one function processEclipseFormat using const Deck& as input argument already in CpGridData.hpp.

A little surprising - I think it should be removed.

GitPaean force-pushed the using_pinched_grid_numerical_aquifer branch from f386555 to 0cfca3b Compare March 26, 2021 23:02

GitPaean mentioned this pull request Mar 26, 2021

Using pinched grid for numerical aquifer OPM/opm-common#2389

Merged

GitPaean commented Apr 6, 2021

View reviewed changes

opm/grid/cpgrid/processEclipseFormat.cpp Outdated Show resolved Hide resolved

GitPaean force-pushed the using_pinched_grid_numerical_aquifer branch 3 times, most recently from 83748dd to 6efd612 Compare April 7, 2021 21:51

GitPaean marked this pull request as ready for review April 7, 2021 22:14

blattms reviewed Apr 8, 2021

View reviewed changes

GitPaean closed this Apr 8, 2021

GitPaean reopened this Apr 8, 2021

GitPaean commented Apr 9, 2021

View reviewed changes

GitPaean force-pushed the using_pinched_grid_numerical_aquifer branch from eb5fc8a to 0300e5d Compare April 9, 2021 22:19

GitPaean commented Apr 10, 2021

View reviewed changes

GitPaean commented Apr 11, 2021

View reviewed changes

GitPaean added 3 commits April 12, 2021 12:36

using pinched grid for numerical aquifers.

99dba94

fixing tests

c75ac49

WIP in using pointers for processEclipseFormat

7ec48db

for EclipseState and Deck.

GitPaean force-pushed the using_pinched_grid_numerical_aquifer branch from 0300e5d to 7ec48db Compare April 12, 2021 10:36

Merge branch 'master' into using_pinched_grid_numerical_aquifer

8a6a79f

using EclipseGrid as the main input for processEclipseFormat

4852daf

GitPaean requested review from blattms and bska April 16, 2021 07:54

GitPaean added this to the Release 2021.04 milestone Apr 16, 2021

GitPaean changed the title ~~WIP in using pinched grid for numerical aquifers.~~ using pinched grid for numerical aquifers. Apr 16, 2021

bska mentioned this pull request Apr 16, 2021

Report pinched cells #520

Merged

updating function description for processEclipseFormat

aef4620

bska reviewed Apr 16, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into using_pinched_gri…

b7799a5

…d_numerical_aquifer

blattms approved these changes Apr 19, 2021

View reviewed changes

joakim-hove mentioned this pull request Apr 19, 2021

Avoid include of Parser.hpp and Deck.hpp #522

Merged

joakim-hove self-requested a review April 19, 2021 08:21

joakim-hove suggested changes Apr 19, 2021

View reviewed changes

GitPaean removed this from the Release 2021.04 milestone Apr 19, 2021

joakim-hove reviewed Apr 19, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into using_pinched_gri…

85d339c

…d_numerical_aquifer

GitPaean force-pushed the using_pinched_grid_numerical_aquifer branch 2 times, most recently from b2f9f96 to a8e70c8 Compare April 20, 2021 07:31

GitPaean closed this Apr 20, 2021

GitPaean reopened this Apr 20, 2021

joakim-hove approved these changes Apr 21, 2021

View reviewed changes

avoiding using Deck explicitly in processEclipseFormat

402718d

GitPaean force-pushed the using_pinched_grid_numerical_aquifer branch from a8e70c8 to 402718d Compare April 21, 2021 07:37

GitPaean closed this Apr 21, 2021

GitPaean reopened this Apr 21, 2021

joakim-hove merged commit e0d4da6 into OPM:master Apr 21, 2021

GitPaean deleted the using_pinched_grid_numerical_aquifer branch April 22, 2021 07:24

		NNCMaps nnc;
		current_view_data_->processEclipseFormat(input_data, nullptr, nullptr, nnc, z_tolerance, remove_ij_boundary, turn_normals);

Conversation

GitPaean commented Mar 25, 2021

Uh oh!

GitPaean commented Mar 26, 2021

Uh oh!

Uh oh!

GitPaean commented Apr 7, 2021

Uh oh!

GitPaean commented Apr 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GitPaean Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GitPaean Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GitPaean Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GitPaean commented Apr 14, 2021

Uh oh!

GitPaean commented Apr 16, 2021

Uh oh!

bska left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blattms commented Apr 19, 2021

Uh oh!

blattms commented Apr 19, 2021

Uh oh!

blattms left a comment

Choose a reason for hiding this comment

Uh oh!

GitPaean commented Apr 19, 2021 • edited by bska Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joakim-hove left a comment

Choose a reason for hiding this comment

Uh oh!

joakim-hove commented Apr 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GitPaean Apr 8, 2021 •

edited

Loading

GitPaean Apr 8, 2021 •

edited

Loading

GitPaean Apr 15, 2021 •

edited

Loading

GitPaean commented Apr 19, 2021 •

edited by bska

Loading