Add methods that calculates face geometry based on cell corners

[totto82]

-The face center is calculated as raw average of the cell corners
-The face area normals is calculated as the projection of the face
corners.

The methods assumes that the cell corners and face nodes are ordered in
a particular way. i.e. as created from eclGrid.

This implementation gives transmissibilities closer to eclipse.

[andlaus]

while I definitely like to have this, wouldn't the better fix (from the perspective of downstream code) be to make the regular element geometries use these face and cell centers? I have to confess that I don't know if this easy to do, though...

[joakim-hove]

Assuming a right-handed i-j system the eclipse ordering is in my opinion

2----3
|    |
0----1

and

6----7
|    |
4----5

i.e. the shift in global index when going 0 → 2 is nx - and not -nx which the supplied drawing might indicate.

I have not looked at the actual implementation; this is probably just a detail in the comment.

[dr-robertk]

I can take a look at this.

[atgeirr]

wouldn't the better fix (from the perspective of downstream code) be to make the regular element geometries use these face and cell centers?

That would probably cause inaccuracies (and at least change all results) in the upscaling codes that depend on the mimetic finite difference method, which should get proper centroids. Of course, we are already abusing the method when applying it to corner point grids, but using the eclipse-type geometries would make it a lot less correct.

[andlaus]

my main concern is that this forces us to use non-standard methods in the default case and thus it will not work for other grids like PolyhedralGrid. maybe a doItCorrectlyInsteadOfMatching = true template parameter should be added to the ´CpGrid` to be able to chose the desired behaviour...

[blattms]

@andlaus Main concern with this PR or with @joakim-hove's suggestion?

If it is with the first, then I disagree. Non-standard behavior should be achieved with non-standard methods. Only in this way users can use CpGrid without surprises. For other grids these methods can be overloaded/defaulted to use the standard behavior which will make the code work. @totto82's way is the correct way to do it.

[atgeirr]

my main concern is that this forces us to use non-standard methods in the default case and thus it will not work for other grids like PolyhedralGrid

A very good point. I don't like your template solution, since the interface is unchanged by the decision to use one calculation or the other it could be a runtime decision. Perhaps passing a bool telling the class which geometry calculation "family" to use would be an easy way to do it?

[andlaus]

@andlaus Main concern with this PR or with @joakim-hove's suggestion?

with the PR in its current form.

Non-standard behaviour should be achieved with non-standard methods.

true, but different Dune grids are allowed to behave slightly differently (If not, what would be the point of multiple implementations? e.g. I'm not sure if all other unstructured dune grids are aware of the difference between the centroid and the center of an element), and for ECL simulations, the behaviour proposed by the present PR is supposed to be the standard behaviour?!

@totto82 is the correct way to do it.

While I prefer a more elegant solution, I have not a big issue to live with it in its present form. Just be aware that this further reduces the value of the Dune grid interface for OPM, though.

@atgeirr wrote:

Perhaps passing a bool telling the class which geometry calculation "family" to use would be an easy way to do it?

that's equally good IMO. It might make the compiler miss a few optimization opportunities, but i don't think it would matter much performance-wise.

[dr-robertk]

@totto82: Please revise the switch case below such that the DUNE Reference Element is used. This allows to exactly extract this kind of information. I have checked the numbers and they are identical.

[atgeirr]

that's equally good IMO. It might make the compiler miss a few optimization opportunities, but i don't think it would matter much performance-wise.

Since all the geometric computations happen at setup time I don't think it would matter. @blattms do you think this would be an acceptable solution? I.e. the numbers would be accessed with the regular Dune grid methods in any case, but depending on a flag at construction the numbers could be either "ECL" or "Normal" numbers.

[dr-robertk]

I think the implementation is ok. However, the GridHelpers in general should be removed or at least revised. But that is outside of the scope of this PR.

[blattms]

@atgeirr Yes that is not only an acceptable solution. Maybe it is even faster as it allows to precompute this stuff (but this shoud be tested.). Moving to the boolshould be a separate PR.

[atgeirr]

@dr-robertk If you are satisfied with your review of this, please merge.

[dr-robertk]

I'm fine with this being merged if there are not further objections.

[andlaus]

go ahead. don't forget to merge OPM/opm-models#144 after this.

[atgeirr]

I think the ewoms and opm-simulators PRs require the current PR, but not vice versa?

[andlaus]

sure. my point was rather that this PR to have any effect, OPM/opm-models#144 must be merged, too. Its true that there's no haste: I've restarted travis on OPM/opm-models#144, we should probably wait for it to finish.