Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MeshHierarchy: Mark internal public functions as internal in the documentation #165

Open
craffael opened this issue Nov 25, 2019 · 3 comments
Assignees

Comments

@craffael
Copy link
Owner

The MeshHierarchy class has five methods which essentially attach a struct for every mesh entity:

  • const std::vector<PointChildInfo> &PointChildInfos(size_type level) const
  • const std::vector<EdgeChildInfo> &EdgeChildInfos(size_type level) const
  • const std::vector<CellChildInfo> &CellChildInfos(size_type level) const
  • const std::vector<ParentInfo> &ParentInfos(size_type level, dim_t codim) const
  • const std::vector<sub_idx_t> &RefinementEdges(size_type level) const

In order to better align with the rest of LehrFEM++ and to also make it more clear that the data values are indeed attached to Mesh entities, I would suggest to use (Codim)MeshDataSets instead of std::vector. I.e. the signature of PointChildInfos could become

const MeshDataSet<PointChildInfo>& PointChildInfos(size_type level) const

To achieve this, MeshHierarchy would of course need to store CodimMeshDataSets internally instead of std::vector, but I don't think that this is a problem.
An Advantage of using MeshDataSets is also that we get automatic bounds checking, i.e. the user gets an error message when he passes e.g. an entity of the wrong codimension or when the entity belongs to the wrong mesh. In addition, the code to access the stored data becomes a bit less verbose:

auto data = mesh_hierarchy.PointChildInfos(1)[mesh.Index(e)];

would become

auto data = mesh_hierarchy.PointChildInfos(1)(e);
@hiptmair
Copy link
Collaborator

All these methods were never meant for the "public" interface of the library, because all the data structures rely on complicated conventions and are mainly meant for "local use". Therefore, I do not consider the conversion to MeshDataSet worth while.

@craffael
Copy link
Owner Author

craffael commented Nov 26, 2019

Aha I see, that wasn't clear to me when I looked at the header file and also @TobiasRohner started using one of the methods without knowing that these are not part of the "public interface". I see two possible ways to improve the current situation:

  1. we group all these methods in the doxygen documentation under "internal methods" and add a note to everyone saying that it's not meant to be used by the user.
  2. we make the methods private and add a friendship declaration to MeshHierarchy so that the current code still compiles.

What do you prefer? Most of these methods are only used by lf::refinement::test::checkFatherChildRelations() or lf::refinement::WriteMatlabLevel().

@hiptmair
Copy link
Collaborator

I advocate the first approach, because access to refinement information should not be restricted too much.
I will take care of adding a warning to the documentation.

@craffael craffael changed the title MeshHierarchy: Return MeshDataSets instead of std::vector MeshHierarchy: Mark internal public functions as internal in the documentation Dec 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants