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

Deprecate DoFHandler initialization interface. #11160

Merged
merged 3 commits into from Nov 18, 2020

Conversation

marcfehling
Copy link
Member

@marcfehling marcfehling commented Nov 10, 2020

Split from #11133 and only addresses the deprecation of the DoFHandler initialization.

Part of #11102 and #10333.

This PR simplifies the initialization interface for the DoFHandler class by keeping the most common way of initialization (DoFHandler(tria) and distribute_dofs(fe)) and deprecating the remaining.

@kronbichler
Copy link
Member

/rebuild

Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get some more feedback before merging this one!

@tjhei
Copy link
Member

tjhei commented Nov 11, 2020

deprecation of the DoFHandler initialization.

Do you want to summarize what the motivation behind this deprecation is? It doesn't seem to be a terrible feature to have...

@marcfehling
Copy link
Member Author

marcfehling commented Nov 11, 2020

My idea behind this deprecation is to remove ambiguity by providing a clear interface. I wanted to separate the initialization process from the actual enumeration of degrees of freedom.

After merging DoFHandler and hp::DoFHandler, we allow setting active fe indices after the initialization of the DoFHandler object. When distributing dofs, we check if the provided finite element collection fits.

When I worked on the parallelization of the hp elements, I introduced the set_fe functions to make sure that active fe indices are valid with the attached collection of elements. Retrospectively, I would not have introduced them and would like to get rid of them.

I do not like about the initialize function that it actually performs two tasks at once: registering a triangulation and enumerating degrees of freedom. I would be okay with leaving a function initialize(triangulation).


Apart from that, the initialize function basically destroys the DoFHandler object and recreates all internal data structures from scratch. To make sure that no reminiscene to the old triangulation is left, I would rather advise users to destroy the whole DoFHandler object and recreate it freshly with the new triangulation in mind. That is the main reason why I want to get rid of the basic DoFHandler() constructor and the initialize functions.

As we are using DoFHandler(Triangulation) and distribute_dofs throughout all examples (with the exception of step-69), I would like to limit the interface to only these two functions.

Copy link
Member

@bangerth bangerth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm for this simplification of the interface. It makes it easier to reason about the state the object is in (e.g., it always has an associated Triangulation object). It also only removes/deprecates functions that are not in wide use.

The only reason, I believe, to have the DoFHandler() constructor and associated initialize(Triangulation) function was so that one can store DoFHandler objects in collections that require default initialization. But the fact that we never seem to do that in the tutorials is a good indication that that's not a common use case, and there is also always the possibility to use a collection of pointers instead.

So I'm in favor.

@tjhei What do you say? I think you were the lone voice of question here.

@peterrum
Copy link
Member

The only reason, I believe, to have the DoFHandler() constructor and associated initialize(Triangulation) function was so that one can store DoFHandler objects in collections that require default initialization.

This comment reminds me that I am using it like this. I have a MGLevelObject<DoFHandler<MatrixType::dim>> dof_handlers; with each DoFHandler initialized with a different triangulation.

@tjhei
Copy link
Member

tjhei commented Nov 14, 2020

I do not like about the initialize function that it actually performs two tasks at once: registering a triangulation and enumerating degrees of freedom. I would be okay with leaving a function initialize(triangulation).

This is a good point. More intuitive would have been a reinit(Triangulation) that only does the first part.

What do you say? I think you were the lone voice of question here.

I am not opposed to this change but wanted to understand the motivation. Every deprecation/removal has downstream effort for at least some users so that has to be considered.

Peter makes a good point though: usage of DoFHandler in containers becomes more difficult without initialize or reinit. Peter, is there a way to still achieve what you are doing? Can we add a way to emplace an object into MGLevelObject? Or would a new reinit option be a good choice?

@peterrum
Copy link
Member

Peter makes a good point though: usage of DoFHandler in containers becomes more difficult without initialize or reinit. Peter, is there a way to still achieve what you are doing? Can we add a way to emplace an object into MGLevelObject? Or would a new reinit option be a good choice?

I can always use a smart pointer (although it is my last choice). My personal use case should be not a problem. However, this will involve with replacing a couple of . with -> in my code and potentially in many user codes (where the type of Triangulation is determined by some input parameters - sorry for seeing this issue so late).

I think

Or would a new reinit option be a good choice?

would be a good idea. What do others think? reinit functions is not my favorite style (since I would prefer that as many internal fields are cost as possible) but this is common in the library.

@marcfehling
Copy link
Member Author

I didn't think of default construction in containers either. I would be okay with leaving the DoFHandler() and introducing a reinit(Triangulation) function.

@marcfehling
Copy link
Member Author

I've provided a patch for the default constructor and a reinit() function. I will squash or drop this commit depending on the general opinion :)

Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks! But I am a bit confused why the constructor that takes a triangulation does not call the reinit function?

doc/news/changes/incompatibilities/20201030Fehling Outdated Show resolved Hide resolved
Copy link
Member

@kronbichler kronbichler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@marcfehling
Copy link
Member Author

Looks good. Thanks! But I am a bit confused why the constructor that takes a triangulation does not call the reinit function?

For some reason I felt like not altering the initializer list in the non-default constructor. It makes sense to use the reinit function here!

Copy link
Member

@tjhei tjhei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy with this change.

@marcfehling
Copy link
Member Author

Thank you for your feedback!

I will squash the commits so we can finally get this merged!

@marcfehling marcfehling force-pushed the deprecate_dofhandler_init branch 2 times, most recently from 2e15da2 to 44ae758 Compare November 17, 2020 19:42
@peterrum
Copy link
Member

peterrum commented Nov 17, 2020

@marcfehling I have replaced the Do not merged label with Reviewed and ready to merge label. Thanks for the efforts!

@peterrum peterrum merged commit 755c2c2 into dealii:master Nov 18, 2020
@marcfehling marcfehling deleted the deprecate_dofhandler_init branch November 18, 2020 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants