dealii / dealii

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

Mapping::get_vertices() uses GeometryInfo::vertices_per_cell #13568

Open nfehn opened 2 years ago

nfehn commented 2 years ago

What happens in the non-hyper-cube case? see also https://github.com/dealii/dealii/issues/13308

peterrum commented 2 years ago

Could you be a bit more precise? Mapping returns boost::container::small_vector< Point< spacedim >, GeometryInfo< dim >::vertices_per_cell >. GeometryInfo< dim >::vertices_per_cell is not the size of the vector but the max size.

nfehn commented 2 years ago

Yes, I mean the return type. So you want to say that we are on the save side with GeometryInfo< dim >::vertices_per_cell, even if this value is defined for hyper-cubes?

bangerth commented 2 years ago

That last argument is the maximal size boost::small_vector can efficiently save. You need to look at what the function actually returns, i.e., what size it sets the vector to.

nfehn commented 2 years ago

thanks. Ok, so there seems to be no problem in using this function for simplex for example? This information is still somewhat implicit here in this discussion.

I think it would be good to explain this in the documentation, and with your help I can also add this if you want.

More generally, I wonder why this function belongs to Mapping at all, and why it is not associated to Triangulation (and e.g. deprecated for mapping?).

bangerth commented 2 years ago

Would it be easier to understand if the return type was

boost::container::small_vector<Point<spacedim>, (1<<dim)>

instead? I would accept a patch to that end.

As for an explanation of what boost::small_vector does, see https://www.boost.org/doc/libs/1_62_0/doc/html/boost/container/small_vector.html

peterrum commented 2 years ago

boost::container::small_vector<Point, (1<<dim)>

Please not. The information (number of max vertices), should become part of ReferenceCell.

bangerth commented 2 years ago

So you'd like to have

  /**
   * Return the maximal number of vertices a `dim`-dimensional cell can have.
   */
  template <int dim>
  constexpr unsigned int max_vertices_per_cell () { return (1<<dim); }

in namespace ReferenceCell and use that in place of the template argument? Or make it a template variable? That would both certainly be ok with me as well.

bangerth commented 2 years ago

(Actually, in namespace ReferenceCells, plural.)

nfehn commented 2 years ago

What I want (as a minimal solution), is to write into the docu "This function assumes the maximum number of vertices of a cell is not larger than 2^dim", if I understood you correctly. Please correct me.

Of course, I would prefer a non-incremental solution even more. One should just not see GeometryInfo in the return type in my opinion, and also not something like (1<<dim). I actually thought about replacing (1<<dim) in the implementation of GeometryInfo by something readable. Why not std::vectoras return type? Why not remove this function (as written above)?

nfehn commented 2 years ago

Please not. The information (number of max vertices), should become part of ReferenceCell.

Would also be a solution for me since this solution is at least clear in terms of documentation.

bangerth commented 2 years ago

boost::small_vector can store arbitrary numbers of elements. It just happens to do so more efficiently for small numbers, where "small" means "at most as many as given by the second template argument". As a consequence, setting that template argument to max_vertices_per_cell <dim> makes sense.