-
Notifications
You must be signed in to change notification settings - Fork 33
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
warm start updates and tied contact patch test #1260
Conversation
/style |
This one should be ready to go! |
contact_.update(cycle_, time_, dt); | ||
if (contact_.haveLagrangeMultipliers()) { | ||
J_offsets_ = mfem::Array<int>({0, displacement_.Size(), displacement_.Size() + contact_.numPressureDofs()}); | ||
J_constraint_ = contact_.jacobianFunction(J_.release()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this call does not take displacement. Are we sure that the contact_ has the updated value for displacement internally at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the updated displacements are provided when contact_.update()
is called on L309. I added some preconditions to the doxygen comments in contact_data.hpp
so it's clear you have to call update()
with the current config if you want the Jacobian contributions to be accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fantastic. This improves robustness for some problems?
Yeah, this helps. It cuts down on the number of Newton iterations needed for all of the contact examples as compared to turning warm start off. |
* For the displacement function, the argument is the input position, the second argument is the time, and the output | ||
* is the value of the component of the displacement. | ||
* | ||
* @note This method must be called prior to completeSetup() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can/should this be checked at runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a good idea. @btalamini is doing a more complete overhaul of this now. Could this check be added to your Dirichlet BC overhaul, Brandon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have these comments all over the place. At some point we can go through and have a check that people are not calling this after completeSetup(), but I think its out of the scope for this.
const mfem::Vector res = (*residual_)(time_ + dt, shape_displacement_, displacement_, acceleration_, | ||
*parameters_[parameter_indices].state...); | ||
|
||
// TODO this copy is required as the sundials solvers do not allow move assignments because of their memory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a TODO or a NOTE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a TODO once mfem/mfem#3531 is addressed, but it doesn't look like that will happen in the near term. I'll change it to a NOTE
StateManager::setMesh(std::move(mesh), "patch_mesh"); | ||
|
||
// #ifdef SERAC_USE_PETSC | ||
// LinearSolverOptions linear_options{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be removed or a note added why its commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note's been added. We should add PETSc back in, but the convergence was slow with these settings.
blt_list_append(TO physics_parallel_test_sources ELEMENTS | ||
contact_patch.cpp | ||
contact_patch_tied.cpp | ||
contact_beam.cpp | ||
IF TRIBOL_FOUND) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blt_list_append(TO physics_parallel_test_sources ELEMENTS | |
contact_patch.cpp | |
contact_patch_tied.cpp | |
contact_beam.cpp | |
IF TRIBOL_FOUND) | |
blt_list_append(TO physics_parallel_test_sources | |
ELEMENTS | |
contact_patch.cpp | |
contact_patch_tied.cpp | |
contact_beam.cpp | |
IF TRIBOL_FOUND) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor suggestions but thanks for the fixes!
This PR adds penalty and Lagrange multiplier contact to the warm start. Additionally, it adds a tied contact patch test that fails if warm start does not account for contact constraints.
Some other small changes:
ContactData::jacobianFunction()
was not using the displacement vector, so that argument has been removed from the methodode_time_point_
totime_
inSolidMechanicsContact
to match what's inSolidMechanics