test failure: test-geometrytype

The test test-geometrytype fails for me with

dim: 0 vertices: 1 guess: 0 real: 0

dim: 1 vertices: 2 guess: 0 real: 0

dim: 2 vertices: 3 ( possibility 0: 1) guess: 1 real: 0

dim: 2 vertices: 4 ( possibility 0: 11) guess: 11 real: 11

dim: 3 vertices: 4 ( possibility 0: 1) guess: 1 real: 0

dim: 3 vertices: 5 ( possibility 0: 11) guess: 11 real: 11

dim: 3 vertices: 6 ( possibility 0: 101) guess: 101 real: 101

dim: 3 vertices: 7 Error: Dune::Exception [guessTopologyId:.../dune/geometry/test/test-geometrytype.cc:57]: Impossible setting

after I put the following change in place:

--- a/dune/geometry/test/test-geometrytype.cc
+++ b/dune/geometry/test/test-geometrytype.cc
@@ -89,6 +89,7 @@ int main()
       catch (Dune::Exception & e)
       {
         std::cout << "Error: " << e.what() << std::endl;
+        return 1;
       }
     }
 }

Without this change, the test would produce tons of such errors all of which would go unnoticed since the test would eventually exit with status 0.

wouldn't be throw; enough and letting C++ handle the expception?

Yes, that, too, would work of course :)

We should bisect and find out what change caused this problem.

The error message looks totally ok to me. What kind of geometry is it, which hat 7 vertices in 3D? There errors seems to be there since 2012, when I wrote this test.

For the sake of completeness, here's what happens if I do not make the test terminate after the first exception.

https://gitlab.dune-project.org/snippets/17

To be quite honest, I really don't understand the purpose of this test or whether the fact that it fails should worry me.

@christi: Is the right fix for this simply to skip combinations of v and d in the loop

  for (int d=0; d<=8; d++)
    for (int v=d+1; v<=(1<<d); v++)

which cannot occur?

I would say yes.

No, it is not the correct fix. The reason for the loop is that we do not know easily which combinations are possible. Thus I use the brute force approach and perform a full search for possible settings for that amount of vertices.

My suggestion is to adapt the code and distinguish between different types of exceptions. Some signal that this is an inadequate test case, some actually report an error.

Okay, I take it you have something like this in mind?

--- a/dune/geometry/test/test-geometrytype.cc
+++ b/dune/geometry/test/test-geometrytype.cc
@@ -8,8 +8,11 @@
 #include <iostream>
 #include <vector>
 
+#include <dune/common/exceptions.hh>
 #include <dune/geometry/type.hh>
 
+class ImpossibleSettingException : public Dune::Exception {};
+
 std::string convBase(unsigned long v, long base)
 {
   const char* digits = "0123456789abcdef";
@@ -51,10 +54,10 @@ unsigned int guessTopologyId(unsigned int dim, unsigned int vertices)
   if (dim == 0 || dim == 1)
     return 0;
   if (vertices < dim+1 || vertices > 1<<dim)
-    DUNE_THROW(Dune::Exception, "IMPOSSIBLE");
+    DUNE_THROW(ImpossibleSettingException, "IMPOSSIBLE");
   guessTopologyId(dim,vertices,2,1,1,ids);
   if (ids.size() == 0)
-    DUNE_THROW(Dune::Exception, "Impossible setting");
+    DUNE_THROW(ImpossibleSettingException, "Impossible setting");
   std::cout << "(";
   for (size_t i=0; i<ids.size(); i++)
     std::cout << " possibility " << i << ": " << convBase(ids[i],2);
@@ -68,12 +71,16 @@ void testGuess(unsigned int dim, unsigned int vertices)
 {
   std::cout << "dim: " << dim;
   std::cout << " vertices: " << vertices << " ";
-  unsigned int id = guessTopologyId(dim, vertices);
-  std::cout << "guess: " << convBase(id, 2);
-  if (dim <= 3) {
-    Dune::GeometryType gt;
-    gt.makeFromVertices(dim, vertices);
-    std::cout << " real:  " << convBase(gt.id(), 2);
+  try {
+    unsigned int id = guessTopologyId(dim, vertices);
+    std::cout << "guess: " << convBase(id, 2);
+    if (dim <= 3) {
+      Dune::GeometryType gt;
+      gt.makeFromVertices(dim, vertices);
+      std::cout << " real:  " << convBase(gt.id(), 2);
+    }
+  } catch (ImpossibleSettingException const &e) {
+    std::cout << "impossible";
   }
   std::cout << std::endl;

What about these exceptions, though?

dim: 5 vertices: 10 ( possibility 0: 10001 possibility 1: 111) Error: Dune::Exception [guessTopologyId:.../dune/geometry/test/test-geometrytype.cc:66]: Too many options

dim: 6 vertices: 11 ( possibility 0: 10001 possibility 1: 111) Error: Dune::Exception [guessTopologyId:.../dune/geometry/test/test-geometrytype.cc:66]: Too many options
dim: 6 vertices: 12 ( possibility 0: 100001 possibility 1: 1011) Error: Dune::Exception [guessTopologyId:.../dune/geometry/test/test-geometrytype.cc:66]: Too many options

etc.

It is well-known that for dim >= 5 the number of corners no longer uniquely describes the geometry type. Take 'pyramid( pyramid( cube( 3 ) ) )' and 'prism( simplex( 4 ) )': Both have 10 corners. Note that these are exactly the two types coughed up in your example for dim = 5.

So: We should not test the conversion from number of corners to geometry type for dim > 4. It does not make sense (at least to me).

There is also no way of avoiding the impossible setting - there simply is no regular 3d geometry type with 7 corners. However, we could return 'none' in this case.

As a final note, I would like to add that arbitrary numbers of corners (> dim) are possible for geometry type 'none'. So there is always a certain non-uniqueness problem. So, another solution could be to iterate over all geometry types with the same number of corners (similar to a std::multimap< unsinged int, GeometryType >).

I think also in this ambiguous case we should have test, which assures a consistent setup. The thing is, that in this case the gt.makeFromVertices can not work and thus it should throw an exception. Our test would make sure that we observe the correct exception.

@pipping in general your patch is in the direction of what I had in mind.

@christi: Currently makeFromVertices() only ever either returns a valid geometry or raises a NotImplemented exception. It could be extended to additionally raise a new exception type for the ambiguous cases and another one for impossible cases (like a 2D object with fewer than 3 vertices). To that end, we'd need

• Names for the two new exception types
• A (non-exhaustive because it would be infinite -- maybe up to dimension 8 as in the test(*)) list of dimensionally ambiguous polytopes.

If we had that, instead of using gt.makeFromVertices only for dim <= 3, it could always be used and we could check if we get the right exception or NotImplemented.

(*) Where does that number come from by the way?

So, again, this is really not my area of expertise, so please excuse me for asking potentially rather stupid questions.

Is a list of numbers of vertices with corresponding polyhedra, which allows one to distinguish the three cases of no mapping, a unique mapping or an ambiguous mapping to a geometry, something that is easily available in the right book so that it could be hardcoded or even something that one could generate with a few lines of code?

@pipping The implementation would be quite trivial (just recurse over all reference elements, ask for the number of vertices and build the inverse lookup table).

The real questions are:

• what kind of interface do you want to have?
• what is the desired complexity of the operation?
• what is your use case?

Especially to the latter point I have not yet heard a good answer. So: What's your interest in this mapping? Is is just the failing test?

Personally, I still think this feature is a utility and can easily be implemented by any user who is sufficiently advanced to need this feature.

Status changed to closed

Status changed to reopened

@martin.nolte: I see. It seems I'm thinking in the wrong direction. I'm indeed only trying to cut down the number of failing tests, I don't have a use case.

Only failing tests? Fixing failing tests is very important otherwise the whole testing is useless. I want to find regressions and not test around failing tests.
I am with Elias, either we implement the list as suggested by Christan or we drop this feature as implicated by Martin. I don't need the feature but couldn't care less keeping it. Has someone a strong opinion towards one side?
If not Elias can choose or whoever is willing to do the work.

I wouldn't mind dropping the feature. Constructing a polyhedron from its number of corners is an ill-posed problem, as everybody in this thread is well aware of. All possible 'solutions' proposed here all seem like putting lipstick on a pig to me.

I agree with Oliver here - if we can't find a good use case, just drop the feature, it looks to me like it causes more trouble than it's worth.

We have been using basically the same (OK, it is simple and not too long...) code in several modules depending on dune-geometry in order to setup the correct geometry from a list of vertices. Once particular point I have never seen anything which is not 2D or 3D. I doubt anybody uses something like this in higher dimensions, where the trouble starts. I think factoring out such code is a good thing, but perhaps we better move it to some utility class and not make it part of the interface. In this case we could easily make clear that it only works for 1D/2D/3D by specializing accordingly.

@gruenich ragarding the failing test. The test does not fail (at least it didn't, the last time I looked into it). It produces warnings. Do this is a different situation.

Christian, you are right.
I like your suggestion to move it to an utility class.

@christi: You're right, the test doesn't actually fail. It generates such large amounts of output containing Error: Dune::Exception, however, see also snippet 17, that even though this comes from exceptions which are caught, a user who's not 100% familiar with the test will think that something's very wrong.

On top of that, because all exceptions are caught (including also NotImplemented which makeFromVertices could throw), there's nothing that the test actually, well, tests. There's no if(...) exit(-1) anywhere. The only way it could fail is if a segfault was generated somehow.

In summary, the test seems confusing and not very useful. I guess that is what originally made me file this bug.

But on to more constructive thinking: Can we agree on a place and a name for the utility header and class?

If nobody has a suggestions, put the header into dune/geometry/utility and do whatever you think is best. When you create a merge request, people might ask for different names.

When I opened this bug, I was confused by the fact that test-geometrytype generates far too many warnings yet exits successfully. Now that I know these warnings are harmless, and given how far removed the corresponding parts of dune are both from my area of expertise and my area of interest, it's safe to say I won't put any more work into this issue.

If anyone else is willing to improve the situation here, that's great. Otherwise, please close this as WONTFIX.

Reassigned to @christi

Mentioned in commit 66f5c61e

Mentioned in merge request !35 (merged)

Mentioned in commit b18b7bb9

Now that !35 (merged) is merged, can this issue be closed (by restricting the test to dim <= 3)?

changed milestone to %DUNE 2.6.0

Works for me.

closed