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

Early deprecate functions in DoFTools #13365

Merged
merged 1 commit into from Feb 17, 2022

Conversation

peterrum
Copy link
Member

@peterrum peterrum commented Feb 12, 2022

... many functions already return the IndexSets directly.

@peterrum peterrum force-pushed the extract_locally_relevant_dofs branch 3 times, most recently from 6d7d5e3 to 89754f6 Compare February 12, 2022 21:29
@peterrum peterrum force-pushed the extract_locally_relevant_dofs branch from 89754f6 to 16238c7 Compare February 13, 2022 05:45
@kronbichler
Copy link
Member

I am not particularly happy about the incompatibility caused by this change. Even though this is only an early deprecation, we will soon get tons of warnings and then error for almost every single deal.II program that uses MPI. I see the point in terms of some other functions in DoFTools, so the interface makes sense and I support it. But I reserve myself against the early deprecation warning already at this stage: I think it should come only when we have gone through all functions with similar interfaces (uniformly return things computed inside function). I think one of the worst things we can do to our users is to randomly deprecate some of the functions for every release, rather than providing a (somewhat) uniform solution which covers most places.

@peterrum peterrum force-pushed the extract_locally_relevant_dofs branch from 16238c7 to 96b1c60 Compare February 13, 2022 13:52
@peterrum
Copy link
Member Author

I am not particularly happy about the incompatibility caused by this change. Even though this is only an early deprecation, we will soon get tons of warnings and then error for almost every single deal.II program that uses MPI. I see the point in terms of some other functions in DoFTools, so the interface makes sense and I support it. But I reserve myself against the early deprecation warning already at this stage: I think it should come only when we have gone through all functions with similar interfaces (uniformly return things computed inside function). I think one of the worst things we can do to our users is to randomly deprecate some of the functions for every release, rather than providing a (somewhat) uniform solution which covers most places.

I have dropped the pragma but would suggest to keep the comment regarding deprecation. Is that fine?

Comment on lines +1867 to +1868
stokes_relevant_set =
DoFTools::extract_locally_relevant_dofs(stokes_dof_handler);
Copy link
Member

Choose a reason for hiding this comment

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

I think in this place the regular RVO will probably not help the change will give rise to (very slightly) slower code than we had before, as we need to call the operator= here from the temporary object. This is different from the case in step-18 you made above where RVO eliminates the copy (on sane compilers).

I think this is justified for a cheap object like IndexSet given the interface uniformity it brings us. Furthermore, I believe we should at some point make the locally_relevant_dofs a member variable of the DoFHandler, given how often we set up and use these objects and how little memory they consume in the grand scheme of things. Then this gets even more clear that we should do it. But I do not think the case is necessarily as low for all functions that merely fill something, so we might need to make a case-by-case decision.

Copy link
Member

Choose a reason for hiding this comment

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

See also #11896.

Copy link
Member

Choose a reason for hiding this comment

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

Separately, the cost of the move assignment is surely immeasurable compared to the cost of building the object. I'm very much in favor of the new interface.

@tamiko tamiko merged commit 9bdea9c into dealii:master Feb 17, 2022
@bangerth
Copy link
Member

Yes, nice. I like these functions a lot better and had wanted to make this change myself for a long time. Thanks for beating me to it!

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

4 participants