Skip to content

Tupek/residual redesign#1354

Merged
tupek2 merged 22 commits intodevelopfrom
tupek/residual_redesign
Apr 14, 2025
Merged

Tupek/residual redesign#1354
tupek2 merged 22 commits intodevelopfrom
tupek/residual_redesign

Conversation

@tupek2
Copy link
Copy Markdown
Collaborator

@tupek2 tupek2 commented Apr 4, 2025

This completes some of the follow ons from the previous review (naming, _, etc.), but also extends and simplifies the use by including a serac::FunctionalResidual, which will eliminate a TON of duplication in the downstream smith new physics. Also, now SolidResidual derives from FunctionalResidual.

@tupek2 tupek2 force-pushed the tupek/residual_redesign branch from d19b179 to 7ae3ea8 Compare April 4, 2025 05:16
@tupek2 tupek2 requested review from btalamini, ebchin and white238 April 4, 2025 14:49
@tupek2 tupek2 force-pushed the tupek/residual_redesign branch from 7ae3ea8 to 3d6591b Compare April 7, 2025 17:01
@tupek2 tupek2 added the ready for review Ready for active inspection by reviewers label Apr 7, 2025
*
*/
template <typename ShapeSpace, typename OutputSpace, typename... parameter_space, int... parameter_indices>
class FunctionalResidual<ShapeSpace, OutputSpace, Parameters<parameter_space...>,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is FunctionalResidual descriptive enough?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I hope the comments help, and I wouldn't really know what other words to use. The idea: its the 'residual' implemented using the serac::Functional utilities. Eventually we'll have a DfemResidual, or some such.

}
for (auto& t : params) {
pointers.push_back(&t);
for (size_t n = 1; n < params.size(); ++n) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is the 0 index being skipped here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added a comment to explain a bit more what it happening here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Actually, I changed it to remove this confusion.

@tupek2 tupek2 force-pushed the tupek/residual_redesign branch from 3d6591b to 4948a0b Compare April 11, 2025 00:03
@tupek2 tupek2 force-pushed the tupek/residual_redesign branch from bca7142 to c8090fb Compare April 11, 2025 16:40
Copy link
Copy Markdown
Member

@btalamini btalamini 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 how streamlined the solid residual class is now!

{
SLIC_ERROR_IF(vjpFields.size() != fields.size(),
"Invalid number of field sensitivities relative to the number of fields");
SLIC_ERROR_IF(vReactions.size() != 1, "Solid mechanics nonlinear system only supports 1 output residual");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Says "Solid mechanics" again. If the size is 0, this error message is technically correct, but probably misleading. Consider checking size == 0 and size > 1 separately.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think this is fixed now

Copy link
Copy Markdown
Member

@ebchin ebchin left a comment

Choose a reason for hiding this comment

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

Thanks @tupek2, these changes are a definite improvement over the current state of the code.

Two things to consider going forward: 1) how this will work with potential replacements for Functional and 2) how it will work with constraints/contact.

@@ -0,0 +1,277 @@
// Copyright (c) 2019-2024, Lawrence Livermore National Security, LLC and
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the naming redundant here? Maybe we can remove test_ from the name, but then it's easily confused with functional_residual.hpp. Hmm, maybe just leave it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

because its hard to tell if a file is a test, example, or library code, I've suggestion we come up with a naming convention. LiDO does something similar, we could adopt their convention, though I'm ok with this one too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be a meeting topic. I have thoughts but wont die on this hill

using VectorSpace = serac::H1<disp_order, dim>;
using DensitySpace = serac::L2<disp_order - 1>;

enum STATE
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

enum class STATE?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

enum class entries are not size_t, so it doesn't achieve what I'm trying to do here, which is simply give names to the indices to improve readability without having to add lies (aka, comments).

VELO
};

enum PAR
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

enum class PAR?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

same, these are indented to be classical enums to index into things with names.

serac::StateManager::newState(VectorSpace{}, "shape_displacement", mesh->tag());
serac::FiniteElementState density = serac::StateManager::newState(DensitySpace{}, "density", mesh->tag());

states = {disp, velo};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can these be moved here, or does serac::StateManager need to know they exist? Though I think there's still a copy required here, so it probably doesn't matter.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

there are all shared_ptr, I'm not worried about the copy and reference counting cost at this high of a level.

protected:
/// @brief Utility to evaluate residual using all fields in vector
template <int... i>
auto evaluateResidual(std::integer_sequence<int, i...>, double time, const std::vector<FieldPtr>& fs) const
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't look like this is used anywhere. Should it be removed?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, removing. It was needed in an earlier iteration of the design, no longer.

class SolidResidual<order, dim, Parameters<parameter_space...>, std::integer_sequence<int, parameter_indices...>>
: public Residual {
template <int order, int dim, typename... InputSpaces>
class SolidResidual<order, dim, Parameters<InputSpaces...>>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How does this work if we create a SolidResidual built on, say, dFEM?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I suppose we could call this SolidFunctionalResidual, when we are there, there will be a DfemResidual, then maybe a SolidDfemResidual based on that. We can do the name changes at that point if we feel like it.

@btalamini btalamini requested a review from lihanyu97 April 14, 2025 16:13
@tupek2 tupek2 merged commit 1785a6b into develop Apr 14, 2025
13 checks passed
@tupek2 tupek2 deleted the tupek/residual_redesign branch April 14, 2025 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for review Ready for active inspection by reviewers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants