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

Add AffineConstraint::add_constraint(). #16103

Merged
merged 4 commits into from Oct 6, 2023

Conversation

bangerth
Copy link
Member

@bangerth bangerth commented Oct 5, 2023

For a long time, I had wished that AffineConstraint had a function to add a whole constraint all at once, rather than doing it via add_line(), add_entry(), set_inhomogeneity(). This has also been mentioned in #334, and it has come up in #15375. This patch implements it.

Rather than write a new test, I decided to just convert a whole bunch of places in functions we use extensively. Also a few of the tutorials.

In the end, I would love to deprecate the three functions mentioned above. I think that interface is just not very good. What do people think about that?

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

I really like this addition. When I learned about the individual functions I found it quite unintuitive that they were not combined in one call. I find the combined function way easier to understand. One question about the documentation.

* ConstraintLine).
* Return whether this object stores a constraint in which one degree
* of freedom is constrained against another degree of freedom (as
* opposed to having only constraints of the form $U_i=42$ where
Copy link
Member

Choose a reason for hiding this comment

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

typically we list contraints as u_i ..., here you capitalize U. Was this on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

I switched it to x_i since that's what the documentation of the AffineConstraints class uses.

@gassmoeller gassmoeller merged commit b9b1677 into dealii:master Oct 6, 2023
15 checks passed
@bangerth bangerth deleted the add-constraint branch October 7, 2023 00:07
Comment on lines +2247 to +2248
for (const auto &dep : dependencies)
add_entry(constrained_dof, dep.first, dep.second);
Copy link
Member

Choose a reason for hiding this comment

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

In light of #16121, I believe this is wrong and should be add_entries(...). I will make a quick test to see if it works, otherwise I have to postpone it because my time is constrained.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a follow-up patch that avoids the repeated calls. Hold on with your timings.

@kronbichler
Copy link
Member

In the end, I would love to deprecate the three functions mentioned above. I think that interface is just not very good. What do people think about that?

I think it would be good to deprecate those functions eventually indeed, or at least limit them to internal use. So this has my support. We will need some time to let users adapt to this change, because it will affect many users.

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

3 participants