Store index/dimension in 8 bits

[mglisse]

Summary of Changes

What dimensions do we want to support in the Triangulation package? The optional policy to store mirror indexes in the full cells of the TDS currently uses an int to store an index. Since indexes are bounded by the dimension (+1), that seems like a waste of memory. Using uint8_t is sufficient up to dimension ~250, reduces the memory overhead of this policy significantly, and improves performance a bit. Is it ok to assume that the dimension fits in 8 bits (with a bit of margin), or do I need to use some meta-programming so it uses different types depending on the dimension?

For a dynamic dimension, having a whole vector for the mirror indexes remains a large overhead even if it is a vector<char>. Since we only need to store ~ d numbers of log(d) bits, a single 64-bit integer should suffice to handle dimensions up to ~ 15 (i.e. the ones I care about). Or the 3 vectors (vertices, neighbors, mirror indices) could be merged into a single vector. And we could reduce a bit the overhead by using unique_ptr<T[]> instead of vector, since the size is fixed at construction. But that's all for later, once I have an idea what dimensions are relevant.

Release Management

• Affected package(s): Triangulation
• License and copyright ownership: unchanged

[lrineau]

You should probably ask Gudhi developers. Are they users of dD CGAL Triangulations?

[mglisse]

You should probably ask Gudhi developers. Are they users of dD CGAL Triangulations?

We use CGAL dD Delaunay/regular triangulations in Gudhi, yes. But I have trouble imagining using it in dimension 200+. As the CGAL user manual shows, inserting 100 points in a Delaunay triangulation of dimension 12 already takes 40 minutes and 7G of memory... (inserting n≤dim affinely independent points should be doable though not interesting, but as soon as we have to do orientation and insphere tests, things will be bad)
We do sometimes use a kernel in high dimension, but then for the tangential complex we locally project points to a low-dimensional subspace and only triangulate there.
I could vaguely imagine someone who has points in high dimension that all sit (exactly?) on a low-dimensional subspace, asking CGAL for a triangulation in the ambient space, expecting to get a low-dimensional complex, but that doesn't seem like a great idea in case there is even a small bit of noise and a point ends up outside the expected affine hull.
I was more curious if there might be other uses of the TDS, not built as Delaunay triangulations but in a faster, combinatorial way where it might still be sensible to work in dimension 300...

[mglisse]

Let me ask some of the authors. @horasio , @odevil , do you have an opinion? In the CGAL Triangulation package, how large a dimension did you intend to support: 15? 250? 60k?

[horasio]

If you give it dimension-250 input, I think the code will seriously consider whether life is worth it, and maybe jump out of the window. To paraphrase B. Gates, 15D ought to be enough. Although I've seen some people playing with CGAL dD-Del in dimension 17, with very very few points. B. Lévy is toying with the idea of 6D Delaunay (phase space in physics). Maybe some people in loop quantum gravity are interested in TDS in ≈10D ?
In summary, I'm siding with Marc. Is it difficult to make your proposal a templated policy choice ?

[mglisse]

Thanks for your comment.

If you give it dimension-250 input, I think the code will seriously consider whether life is worth it, and maybe jump out of the window.

Delaunay, yes, but it may be possible to build a TDS manually?

Although I've seen some people playing with CGAL dD-Del in dimension 17, with very very few points.

Ok so we should at least support dim 17.

Is it difficult to make your proposal a templated policy choice ?

The particular change in this PR is mostly useful for static dimension, and thus it could relatively easily be done automatically (if static dimension at most 255, use uint8_t, else back to int), no need for a new policy.
Further changes could indeed be done through a new policy TDS_full_cell_mirror_with_dim_at_most_15_storage_policy for instance.
I guess I mostly wanted to know if it was worth complicating the code and user experience for the questionable benefit of high dimensions, and where we put the threshold in this balance.

[sloriot]

@mglisse CGAL-6.0-Ic-172 has runtime errors in Triangulation test but I'm not sure it is this PR in particular

terminate called after throwing an instance of 'CGAL::Precondition_exception'
  what():  CGAL ERROR: precondition violation!
Expr: 0<=i && i<=maximal_dimension()
File: /mnt/testsuite/include/CGAL/Triangulation_ds_full_cell.h
Line: 111

[mglisse]

@mglisse CGAL-6.0-Ic-172 has runtime errors in Triangulation test but I'm not sure it is this PR in particular

Indeed, value -1 is used sometimes, in a way that would require further tweaks to work with unsigned storage, so I just switched to signed, i.e. dimensions up to ~120.

(I don't think it fundamentally changes the issue, either we consider that any dimension above 20 is crazy, or we don't and I have to add a condition and it might as well test for 127 instead of 255.)

[afabri]

I would also say go for it. At worst this will trigger an issue of a user who needs more, so we will even learn about this kind of use case. And we can offer this user an easy fix. But you have to document it, and also as breaking change in changes,md

[mglisse]

But you have to document it, and also as breaking change in changes,md

Done, though this means this branch now probably conflicts with the other one that touches changes.md.

[sloriot]

triggers:

/mnt/testsuite/include/CGAL/TDS_full_cell_mirror_storage_policy.h:45:29: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
   45 |         mirror_vertices_[i] = static_cast<std::int_least8_t>(index);

here and here for example.

[mglisse]

-Wstringop-overflow and -Warray-bounds are 2 of the worst offenders among the recent gcc warnings that only ever give false positives...

[sloriot]

Except from the aforementioned warning (that is a false positive according to Marc), successfully tested in CGA-6.0-Ic-173