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

[GeoMechanicsApplication] Cleaned up "geo" truss and cable elements #12543

Merged
merged 14 commits into from
Jul 15, 2024

Conversation

avdg81
Copy link
Contributor

@avdg81 avdg81 commented Jul 15, 2024

📝 Description
Eliminated an unnecessary intermediate base class to make the element class hierarchy simpler. Also removed some duplicated code and fixed a few problems that were found by our code quality tools.

🆕 Changelog

  • Removed intermediate base class GeoTrussElementLinearBase. Class GeoLinearTrussElement now has class GeoTrussElementBase as its base class.
  • Moved several member functions from the no longer existing class GeoTrussElementLinearBase to class GeoLinearTrussElement.
  • Removed the explicitly defined destructor of class GeoLinearTrussElement to follow the rule of zero.
  • Made some data members private (which used to be protected).
  • The default implementation of virtual member function UpdateInternalForces (of class GeoTrussElementBase) now throws an exception. The previous implementation was overridden by both derived classes and it also wasn't called directly. As a result, the code was never run. To express more clearly that the default implementation is not supposed to be used, it now throws an exception.
  • Removed a few empty statements.
  • Replaced some duplicated code for the cable element by calling a base member function.

avdg81 added 12 commits July 15, 2024 14:16
By removing the duplicated code it becomes clear what the cable element
does in addition to what the truss element (its base class) does.
Effectively, the `override` was never executed, since the one and only
derived class provided its own `override` that does not reference the
one of the base class.
Previously, the default implementation was overridden by all derived
classes (and it was not explicitly referenced from anywhere else). To
make clear the code was never executed, it has now been replaced by
throwing an exception.
These elements were untested. For truss elements in 3D models, we can
use the truss element of StructuralMechanicsApplication. For truss
elements in plane strain models, we need to investigate their need
first.
The moved member functions were only used by the derived class. This is
to prepare for the removal of the base class.
This is to prepare for the removal of the base class.
The moved member functions were only used by the derived class. This is
to prepare for the removal of the base class.
The moved member functions were only used by the derived class. This is
to prepare for the removal of the base class.
This is to prepare for the removal of `GeoTrussElementLinearBase`. It is
no longer needed, since `GeoLinearTrussElement` was the only class that
was derived from it.
The class had become redundant and it was no longer referenced.
- Applied the rule of zero by removing an explicitly defined destructor.
- Removed an empty statement.
@avdg81 avdg81 requested a review from rfaasse July 15, 2024 12:39
@avdg81 avdg81 self-assigned this Jul 15, 2024
@avdg81 avdg81 added GeoMechanics Issues related to the GeoMechanicsApplication Cleanup labels Jul 15, 2024
@avdg81 avdg81 marked this pull request as ready for review July 15, 2024 13:01
rfaasse
rfaasse previously approved these changes Jul 15, 2024
Copy link
Contributor

@rfaasse rfaasse left a comment

Choose a reason for hiding this comment

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

This is a nicely scoped clean-up and removes quite some duplication. I checked that the implementations of the derived classes are identical now that some of the structure has changed.

I only have a single question about if there might be more possibilities to clean-up the cable, but I leave it up to you if that is worth it, or if we just rely on the new way of doing reset displacement (since that should just work out of the box with the cable) and remove it after.

}

BaseType::CreateElementStiffnessMatrix(rLocalStiffnessMatrix, rCurrentProcessInfo);
Copy link
Contributor

Choose a reason for hiding this comment

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

This clarifies a lot indeed, do you think this is possible for other functions in the class as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I did not check that, but I can imagine there are more such functions. I would suggest to investigate that separately and to create a new PR when needed.

@avdg81 avdg81 enabled auto-merge (squash) July 15, 2024 15:11
@avdg81 avdg81 merged commit 634ad23 into master Jul 15, 2024
9 of 11 checks passed
@avdg81 avdg81 deleted the geo/clean-up-geo-structural-elements branch July 15, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup GeoMechanics Issues related to the GeoMechanicsApplication
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants