Split the id interface into two functions

[SoilRos]

The id method is being overloaded for two cases, one for the Entity and another one for EntityRep. The problem is that the two overloads do vastly different things and since the Entity inherits from EntityRep, it is likely that a cast to the base case changes the behavior without anyone noticing. This patch splits the id function into id and idLevelZero to clarify that the id function is only valid for full entities of type Entity and not its base class EntityRep.

This is a follow up PR from #949 where test on ids on Entity and EntityRep are compared. This supersedes #902, #905, and #897 which had similar goals.

[SoilRos]

jenkins build this serial please

[Copilot]

Pull request overview

This PR refactors the ID interface to prevent confusion between Entity and EntityRep types by splitting the overloaded id() method into two distinct methods: id() for full Entity objects and idLevelZero() for lightweight EntityRep objects. Since Entity inherits from EntityRep, the previous design could lead to unintended behavior changes through implicit casting. This change also removes support for codimension 1 (faces) from the ID and index systems.

Changes:

• Split id() method into id() (for Entity) and idLevelZero() (for EntityRep) to clarify semantics
• Remove codimension 1 support from Codim template specializations and subIndex/subId methods
• Update all call sites in CpGridData.cpp to use idLevelZero() for EntityRep objects
• Improve error handling in Entity::subEntities() by throwing exception instead of silently returning 0

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/cpgrid/lgr/id_entity_entityrep_test.cpp	Updated test to use idLevelZero() for EntityRep
tests/cpgrid/distribution_test.cpp	Commented out codim 1 face test, whitespace cleanup
opm/grid/cpgrid/Indexsets.hpp	Split id interface, added deleted overloads, restricted Codim to codim 0 and 3
opm/grid/cpgrid/Indexsets.cpp	Removed codimension 1 from subIndex and subId switch statements
opm/grid/cpgrid/Entity.hpp	Restricted Codim to codim 0 and 3, improved subEntities error handling
opm/grid/cpgrid/CpGridData.cpp	Updated all EntityRep id calls to use idLevelZero()

Comments suppressed due to low confidence (3)

opm/grid/cpgrid/Indexsets.hpp:249

• Typo in error message: "thatn" should be "than".

                                  "IdSet::id not implemented for codims other thatn 0, and 3.");

tests/cpgrid/distribution_test.cpp:506

• This test for codimension 1 entities (faces) has been commented out, but it's unclear why. Since codimension 1 support has been removed from the IdSet, this test should either be deleted entirely or replaced with a comment explaining that faces (codim 1) are no longer supported in the ID system. Leaving commented-out code without explanation reduces code maintainability.

                /*
                BOOST_REQUIRE(idSet.id(Dune::createEntity<1>(grid, face, true)) ==
                              seqIdSet.id(Dune::createEntity<1>(seqGrid, seqFace, true)));
                */

opm/grid/cpgrid/Indexsets.hpp:77

• The Codim template specializations were changed from 'using' to 'typedef' syntax. This creates an inconsistency with Entity.hpp which uses 'using' syntax for the same purpose. For consistency within the codebase and alignment with modern C++ style, consider keeping the 'using' syntax instead of 'typedef'.

            struct Codim;

            template <>
            struct Codim<0>
            {
              typedef cpgrid::Entity<0> Entity;
            };

            template <>
            struct Codim<3>
            {
              typedef cpgrid::Entity<3> Entity;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bska]

Is there a reason you created a branch in the main OPM-Grid repository instead of your own fork?

[SoilRos]

@bska I didn't notice. My default upstream always points to OPM directly. I will see how to change this. Should I delete and do it in my fork?

[bska]

My default upstream always points to OPM directly. I will see how to change this

For what it's worth, I (usually) configure two remotes for my work trees, one is my personal fork at GitHub which I typically refer to as "origin" and the other is the main GitHub repository which I typically refer to as "upstream". Then I run the following command:

git remote set-url --push upstream PUSH-DISALLOWED

Since the string PUSH-DISALLOWED is not a valid URL, I always get some kind of (more or less) meaningful diagnostic if I inadvertently try to push to the "upstream" instead of the "origin".

Should I delete and do it in my fork?

I think it's okay to keep the current branch and PR until it's approved/merged, but try to avoid pushing directly to the main repository in the future.

[SoilRos]

jenkins build this serial please

[SoilRos]

I wanted to specialize the Codim struct to fit the dune 2.10 interface (missing Codim should be identified via SFINAE), but full specialization on the member classes does not work. There are other ways to achieve this, but I just reverted it and will propose it in another PR to keep discussion focused on the id interface.

[SoilRos]

@bska thanks for the detailed explanation. I just set it up the push URLs on my end.

[SoilRos]

jenkins build this serial please

[akva2]

any chance you can address

/build/default/install/include/opm/grid/cpgrid/Entity.hpp:98:7: warning: declaration of 'using Dune::cpgrid::Entity<<anonymous> >::Codim<cd>::Entity = class Dune::cpgrid::Entity<cd>' changes meaning of 'Entity' [-Wchanges-meaning]
   98 |         using Entity = ::Dune::cpgrid::Entity<cd>;
      |       ^ ~~~~
/build/default/install/include/opm/grid/cpgrid/Entity.hpp:98:16: note: used here to mean 'class Dune::cpgrid::Entity<cd>'
   98 |         using Entity = ::Dune::cpgrid::Entity<cd>;
      |                ^~~~~~~~~~~
/build/default/install/include/opm/grid/cpgrid/Entity.hpp:71:7: note: declared here
   71 | class Entity : public EntityRep<codim>

[SoilRos]

jenkins build this serial please

[SoilRos]

Note that the CI fails due dune-grid < 2.11 not being able to write grids without the codim 1 entity. I already fixed this in master: https://gitlab.dune-project.org/core/dune-grid/-/merge_requests/793.

[SoilRos]

jenkins build this serial please

[SoilRos]

jenkins build this serial please

[SoilRos]

@akva2 commit 4a1c090 should address that.

[akva2]

brilliant, thx. we will be moving to dune 2.11 soon so your fighting old code will come to an end.

[SoilRos]

jenkins build this serial please