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

Rename ShapeInfo::lexicographic_numbering and clarify documentation #16249

Open
nfehn opened this issue Nov 7, 2023 · 6 comments
Open

Rename ShapeInfo::lexicographic_numbering and clarify documentation #16249

nfehn opened this issue Nov 7, 2023 · 6 comments

Comments

@nfehn
Copy link
Contributor

nfehn commented Nov 7, 2023

As discussed here exadg/exadg#589 (comment), I believe the name lexicographic_numbering (https://dealii.org/current/doxygen/deal.II/structinternal_1_1MatrixFreeFunctions_1_1ShapeInfo.html#abad0471d12c4f07f3d83271377e24231) should be changed to lexicographic_to_hierarchical_numbering, and the documentation should be clarified in terms of index/key and data of the std::vector.

@kronbichler
Copy link
Member

Regarding the name (which was given by me), I agree, but we probably need to make the solution backward compatible because those names might potentially be used in other codes. We could argue that this is in the internal namespace and thus one could skip it.

For the documentation, extending the description would certainly be helpful. Do you want to make a suggestion?

@nfehn
Copy link
Contributor Author

nfehn commented Nov 7, 2023

I understand the point with backward compatibility. A problem I see is that lexicographic_numbering is not a member function, but a member variable (data). From a design perspective, the following problems occur according to my understanding:

  • In case of a function, backward compatibility would be straight-forward to me (introduce a new function with better name, let the old function call the new one to remain backward compatible).
  • In case of member variables, we can not (better: must not) introduce a second member variable with a different name but the same data, since this would introduce redundancies in data which are to be avoided in any case.
  • I could introduce a getter function with a name I consider appropriate. However, this is against the general design of ShapeInfo as a struct (so kind of a class with public member variables), such that ambiguities/irritations are also introduced in this case.

I remember other discussions where we argued that internal "may" be used in application code, but one does not have the guarantee of backward compatibility. Given the reasoning above and being aware of the important topic of backward compatibility, I nevertheless believe we should break it here.

@kronbichler
Copy link
Member

These are good points, we should not introduce a second member variable with a copy of the same data. What I believe we have done in the past is to add a reference variable to the same data (that gets the declaration deprecated), i.e., we would introduce const std::vector<unsigned int> &lexicographic_numbering that gets, in the constructor, a reference to the new variable lexicographic_to_hierarchic_numbering. I can try that if you agree (and rather have me do it), but I would appreciate help with the documentation.

@peterrum
Copy link
Member

peterrum commented Nov 7, 2023

What I believe we have done in the past is to add a reference variable to the same data (that gets the declaration deprecated), i.e., we would introduce const std::vector &lexicographic_numbering that gets, in the constructor, a reference to the new variable lexicographic_to_hierarchic_numbering

@kronbichler Exactly. See e059d9f. But don't forget to implement the copy-assignment/constructor, since the reference will disable the implicit ones.

@nfehn
Copy link
Contributor Author

nfehn commented Nov 8, 2023

Seems to be a solution that has downsides as well but is at least backward compatible.

@nfehn
Copy link
Contributor Author

nfehn commented Dec 4, 2023

For the documentation, extending the description would certainly be helpful. Do you want to make a suggestion?

I think I would skip "deal.II's numbering" and replace the first sentence e.g. by

"This function generates the map from the lexicographic to the hierarchical numbering."

In the second setence, I would skip "a lexicographic numbering of".

Having looked at the deal.II documentation related to this topic, I could imagine a link to https://dealii.org/current/doxygen/deal.II/namespaceFETools.html#adce51f339387fc6d4a88be9ca0d31746 would actually be more helpful than to FEEvaluation or DoFHandler as done currently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants