faceNormal of UnstructuredGrid scaled with faceArea.

[dr-robertk]

The faceNormals in UnstructuredGrid are scaled with the faceAreas which is counter intuitive, because then faceNormal is not a normal anymore but a vector orthogonal to the face scaled with it' area. This is usually what is needed numerical solvers but it should be called normal.

[atgeirr]

I think it is still ok to call it a normal, just not a unit normal. While it promises a little speed in the quite common case where the product of the unit normal and face area is required, I am not a big fan of the convention myself. I do not think it is trivial to change though, for the following reasons:

• plenty of existing code depend on the convention
• MRST has the exact same convention (which historically is the reason the UnstructuredGrid has it).

In the future, I think perhaps we should use the term unit normal by default for new interfaces we write because it is more precise (and of course those should not be pre-multiplied by face area).

This is usually what is needed numerical solvers but it should be called normal.

Did you mean that it should not be called normal?

[bska]

The face_normals in UnstructuredGrid are scaled with the face_areas

For the record, that's intentional.

then faceNormal is not a normal anymore

❓

That assigns more meaning to the word normal than I'm used to.

[dr-robertk]

@atgeirr: I just wanted to point out that the two implementations, CpGrid and UnstructuredGrid, do something different here and it leads to trouble, as for example, experienced in opm-autodiff GeoProps.

@bska: I know that it is intentional, it's in the docu of UnstructuredGrid. The thing that puzzles me is that it was done the other way around in CpGrid (by supposedly the same guys).

But I'm pretty sure you guys can figure out a solution for this. I actually done care all to much because I don't think that the GridHelpers will survive much longer.

[bska]

The thing that puzzles me is that it was done the other way around in CpGrid (by supposedly the same guys).

Convenience for the implementation of Dune interface method Intersection::unitOuterNormal()

[dr-robertk]

I hope we agree on the problem that the function GridHelpers::faceNormal should give the same answer for both grids, right? Then the question is, which of the grid implementations needs to change? Or do we need a second method, e.g. faceUnitNormal?

[atgeirr]

The thing that puzzles me is that it was done the other way around in CpGrid (by supposedly the same guys).

The CpGrid was started before the UnstructuredGrid was created, and at the time I was not sufficiently into MRST to think anything but unit vectors are called for.

I hope we agree on the problem that the function GridHelpers::faceNormal should give the same answer for both grids, right? Then the question is, which of the grid implementations needs to change? Or do we need a second method, e.g. faceUnitNormal?

Perhaps this is a solution:

• rename the function GridHelpers::scaledFaceNormal()
• change it to return std::array<double, 3> (to avoid introducing new dependencies)
• make the CpGrid version return scaled and not unit normals

The second point above is necessary to return something not stored in the grid itself (scaled normals for CpGrid).

[blattms]

On Wed, Jul 08, 2015 at 04:55:07AM -0700, dr-robertk wrote:

I hope we agree on the problem that the function
GridHelpers::faceNormal should give the same answer for both grids,
right?

No. Strictly spealing, faceNormal only gives a direction and does not
imply any given length. I always worked around this problem using
template specialization/function overloading in opm-core.

Then the question is, which of the grid implementations needs to change?
Or do we need a second method, e.g. faceUnitNormal?

If we do that then a normal scaled by the area would be handy, too. Seems
like a good idea for clearer code.

[atgeirr]

We have in other contexts discussed other minor improvements to the function-based grid interface. Then it was said (by whom I am not sure) that we should not make any effort to improve it as it is an interim solution. I am not sure if that holds, so I'll keep this issue open as a reminder.