Changing IdType to int64_t

[kjetilly]

This pull request changes the IdType to int64_t to avoid some overflow errors seen previously.

No change in opm-simulators is needed.

[atgeirr]

The extra template argument that causes the need for a downstream PR: is there a way to avoid needing to specify the argument, having it deduced instead?

Otherwise I think this looks like a reasonable direction, but I have not done a thorough review as this is still in draft mode. I think replacing is_same_v<EntityType, unsigned int> with is_integral_v<EntityType> is a good thing, this was already noticed as something to fix.

[kjetilly]

The extra template argument that causes the need for a downstream PR: is there a way to avoid needing to specify the argument, having it deduced instead?

@atgeirr I think this can be avoided. One solution would be to exchange the order of the template arguments, ie. having IdType the last argument to allow for automatic deduction, but that would not make it compatible with the overloaded version taking non-integral types (which already is <EntityType, GridType>).

Another solution would be to make the functions have a version taking auto arguments that then dispatches the function with the correct template arguments, or using said auto function and just using a if constexpr to deal with the different cases. The latter would probably entail less code and might be easier to read. It would go something like:

template<typename Grid, typename GridView>
template<class GridType>
int Opm::LookUpData<Grid,GridView>::getFieldPropIdx(const auto& elem) const
{ // ignoring the check for GridType to keep it simple, but that could either be handled as is or with a constexpr if.
    if constexpr (!std::is_integral_v(decltype(elem)) {
        static_assert(std::is_same_v<Grid,GridType>);
        static_assert(std::is_same_v<EntityType,Dune::cpgrid::Entity<0>>);
        if (isFieldPropInLgr_ && elem.level()) { // level > 0 == true ; level == 0 == false
            // In case some LGRs do not have refined field properties, the next line need to be modified.
            return elem.getLevelElem().index();
        }
        else {
            return elem.getOrigin().index();
        }
    } else {
      static_assert(std::is_same_v<Grid,GridType>);
      // Check there are no LGRs. LGRs (level>0) only supported for CpGrid.
      assert(gridView_.grid().maxLevel() == 0);
      return elem;
  }
}

Now, this would also introduce change in opm-simulators I think, since it would remove the template argument from the non-integer version.

In terms of minimisation of change, I think exchanging the order of the template arguments would be the winner, but that would lead to an inconsistency.

[atgeirr]

Now I think that maybe changing the interface would be better in the end, if we could avoid having to specify (more than one) template arguments, despite a few downstream changes. I think there are confusingly many variants of these functions, which I think is because there are several dimensions here: CpGrid or not, Entity or not. Could we do with just a single one using if constexpr?

[kjetilly]

@atgeirr I've added a sketch with a simplified function signature. Note that I still have to add documentation to the new functions to properly document this. The function signatures are a bit cleaner, though I would argue that their definitions might be slightly more cumbersome to read (a lot of if else).

Also note that there are some simplifications that can be done, since some of the functions coincide. Right now this is still handled as separate if-branches to make it easier to compare to the original code. Will change this before I take it out of draft.

[kjetilly]

So it seems a fortunate side effect of this edition is that no change in opm-simulators is needed.

[kjetilly]

Jenkins build this please

[atgeirr]

I think this looks like a good reduction of complexity. @aritorto as the original author of this code, do you agree?

[aritorto]

Indeed, nice simplification/improvement. Why is it still in draft mode? Now running a simulation with CARFIN keyword has been incorporated into master (OPM/opm-simulators#5903). This is (one test) where lookupdata would do the not trivial thing. So, just in case, we could check - one more time - that nothing breaks

[aritorto]

jenkins build this please

[kjetilly]

@aritorto the main reason it's still in draft is that I haven't completed the new documentation of the functions. I will do that now and take it out of draft.

[aritorto]

Good point, @kjetilly (Also it was in the description, my bad...). Documentation is also important. Great!

[akva2]

this requires c++-20.

[atgeirr]

this requires c++-20.

Which part of it?

[akva2]

see jenkins console output and gcc tells you all you need to know.

/var/lib/jenkins/workspace/opm-grid-PR-builder/mpi/install/include/opm/grid/LookUpData.hh:86:27: warning: use of ‘auto’ in parameter declaration only available with ‘-std=c++20’ or ‘-fconcepts’
   86 |     auto operator()(const auto& elementOrIdx, const auto& fieldProp) const;

[kjetilly]

Jenkins build this please

[kjetilly]

Jenkins build this please

[aritorto]

It's a nice simplification. I had only a couple of comments/questions

[kjetilly]

Jenkins build this please

[aritorto]

@kjetilly The PR description needs a small update : ) If @atgeirr and @akva2 have no further comments, it can be merged. Nice improvements!

[blattms]

The companion PR in opm-simulators is still WIP. I guess that needs to be merged with one?

[blattms]

Jenkins build this please

[kjetilly]

The companion PR in opm-simulators is still WIP. I guess that needs to be merged with one?

This was a bit unclear since I did not close the opm-simulators PR, but in its current version, this PR does not need any change outside of opm-grid. I closed the opm-simulators PR now since it is no longer needed.