dealii / dealii

The development repository for the deal.II finite element library
https://www.dealii.org
Other
1.34k stars 741 forks source link

CellIDTranslator for more than 10 mesh levels and 32 bit integers in 3D? #12337

Closed kronbichler closed 1 year ago

kronbichler commented 3 years ago

Looking at the internal class https://github.com/dealii/dealii/blob/2fd0379961c8fe6b3195795f349c3cccf71aab79/include/deal.II/multigrid/mg_transfer_global_coarsening.templates.h#L453-L454 I am wondering why it uses the global_cell_index and not unconditionally uint64_t. With a 3D mesh, one could easily imagine a case with much fewer cells than a few millions, but more than 11 global levels, for which the code https://github.com/dealii/dealii/blob/2fd0379961c8fe6b3195795f349c3cccf71aab79/include/deal.II/multigrid/mg_transfer_global_coarsening.templates.h#L463-L467 will overflow. The case is probably not that common, but it surely exists, and it seems we are only artificially limiting ourselves as the 64 bit data does not really show up prominently (e.g. in a memory consumption).

peterrum commented 3 years ago

Let me just add one point to the discussion: it might be also something to think about to add a version of DictionaryPayLoad built around of a map instead of a vector. The memory consumption in the case of very locally refined meshes is too much; but on the other hand one might want to prefer local smoothing in these cases...

peterrum commented 3 years ago

@kronbichler PR #12513 adds an assert that recommends to configure deal.II with 64bit indices. For more, one needs to (significantly) more work. In particular, within the global coarsening algorithm ComputeIndexOwner is used which is built around index sets...

kronbichler commented 2 years ago

I think this one has been worked around by some other means, hasn't it @peterrum ?

peterrum commented 2 years ago

I have only added an assert that one should compile deal.II with 64bit indices, since the class operates on types::global_cell_index:

https://github.com/dealii/dealii/blob/7e1892d3381e93ad2af8d5c2f3a51af23bef2ac1/include/deal.II/grid/cell_id_translator.h#L136-L148

One could, work always on 64bit integers as done here:

https://github.com/dealii/dealii/blob/7e1892d3381e93ad2af8d5c2f3a51af23bef2ac1/include/deal.II/grid/cell_id_translator.h#L136-L148

But this would mean that we need to template the classes for index-owner computations:

https://github.com/dealii/dealii/blob/7e1892d3381e93ad2af8d5c2f3a51af23bef2ac1/include/deal.II/base/mpi_compute_index_owner_internal.h#L78-L81

@kronbichler Do you think it is worth to make this change? And to answer your question: the other PRs only targeted the reduction in memory consumption but would not prevent overflow.

kronbichler commented 2 years ago

I think we should be fine with the assertion, the workaround (enable 64 bit integers), and keeping the current non-templated code. If one works with those still reasonably large problems, going to 64 bit integers does not seem to be outrageous, unless the work on our side is very low (which it does not seem at first sight).

kronbichler commented 2 years ago

Adjusting the target milestone.

bangerth commented 1 year ago

Can we close this one? The assertion has been added, and I think that going beyond that would require work nobody is currently motivated to do.

kronbichler commented 1 year ago

Yes, let us close.