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] Extract computation of integration coefficients #12334

Merged
merged 10 commits into from
May 1, 2024

Conversation

avdg81
Copy link
Contributor

@avdg81 avdg81 commented Apr 30, 2024

📝 Description
Extracted the computation of a vector of integration points from the integration point loops in (virtually) all implementations of CalculateAll. Also modified a few function parameters types from reference to reference-to-const. In that way, it becomes clearer that the supplied object won't be modified by the callee.

@avdg81 avdg81 added Refactor When code is moved or rewrote keeping the same behavior GeoMechanics Issues related to the GeoMechanicsApplication labels Apr 30, 2024
@avdg81 avdg81 requested review from rfaasse and WPK4FEM April 30, 2024 15:38
@avdg81 avdg81 self-assigned this Apr 30, 2024
@avdg81 avdg81 requested a review from markelov208 May 1, 2024 07:09
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.

Nice PR, quite some changes, but a very precise scope! I have one question left, I believe we discussed it already, but just double checking before approving

@rfaasse
Copy link
Contributor

rfaasse commented May 1, 2024

@markelov208 FYI, this is Annes PR for making the lists for the integration coefficients. In the end, lists were only added for the CalculateAll functions, so you could follow a similar way in the CalculateMassMatrix function (note that the signature of the CalculateIntegrationCoefficient has changed, so it's a good idea to bring your branch up to date with master when this has merged).

Copy link
Contributor

@markelov208 markelov208 left a comment

Choose a reason for hiding this comment

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

Hi Anne, nice work: a single point PR, a step forward to change from the integration point level to the element level and to get rid of ElementVariables. I have only one question: why not to create a static function to calculate integration_coefficients?

@avdg81
Copy link
Contributor Author

avdg81 commented May 1, 2024

Hi Anne, nice work: a single point PR, a step forward to change from the integration point level to the element level and to get rid of ElementVariables. I have only one question: why not to create a static function to calculate integration_coefficients?

Hi Gennady, good question. I have considered that, but then I realized that this is only an intermediate step. The aim is to have such a function available later, perhaps as part of an ElementGeometryPolicy. There is an ongoing discussion with @rfaasse to see how to best fit this in. If you have any ideas or suggestions yourself, feel free to share them with us. Thanks!

@markelov208
Copy link
Contributor

Hi Anne, nice work: a single point PR, a step forward to change from the integration point level to the element level and to get rid of ElementVariables. I have only one question: why not to create a static function to calculate integration_coefficients?

Hi Gennady, good question. I have considered that, but then I realized that this is only an intermediate step. The aim is to have such a function available later, perhaps as part of an ElementGeometryPolicy. There is an ongoing discussion with @rfaasse to see how to best fit this in. If you have any ideas or suggestions yourself, feel free to share them with us. Thanks!

Hi Anne, thank you for the clarification. Can we put such a function into 'geometry.h'? The 'geometry' class has already integration points. I have personal interest in this function ;) to use it for CalculateMassMatrix; of course I can copy the statement. Kind regards,

markelov208
markelov208 previously approved these changes May 1, 2024
WPK4FEM
WPK4FEM previously approved these changes May 1, 2024
Copy link
Contributor

@WPK4FEM WPK4FEM left a comment

Choose a reason for hiding this comment

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

The number of times you had to do the same action reveals how many copies exist within our code base. Your change looks to be consequently executed for all of them.
Remarks that I would like to make would without exception refer to the existing situation that you start improving here, so they would not be productive here.

Please go ahead and merge this, such that a next step can be taken.

Comment on lines -471 to -476
// Compute weighting coefficient for integration
Variables.IntegrationCoefficient =
this->CalculateIntegrationCoefficient(IntegrationPoints, GPoint, Variables.detJ);
Variables.IntegrationCoefficient = integration_coefficients[GPoint];

Variables.IntegrationCoefficientInitialConfiguration = this->CalculateIntegrationCoefficient(
IntegrationPoints, GPoint, Variables.detJInitialConfiguration);
Copy link
Contributor

Choose a reason for hiding this comment

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

This piece of code seems to use the same function with the same input. Therefore I think that detJ == detJInitialConfiguration. In the new implementation storing the list of detJ for the integrationpoints would then avoid the second call to CalculateIntegrationCoefficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. ElementVariables::detJ is taken from ElementVariables::detJContainer, which is computed by calling member InitializeElementVariables. ElementVariables::detJInitialConfiguration on the other hand is computed by calling member CalculateKinematics (which in turn calls member CalculateDerivativesOnInitialConfiguration). So they are probably two different things. However, it is unclear to me where ElementVariables::detJInitialConfiguration is being used and whether that is correct. To be investigated.

avdg81 added 2 commits May 1, 2024 15:15
Calculation of the integration coefficient does not require the member
function to be non-`const`. Also, these member functions were declared
`virtual`, but never overridden.
To reduce the amount of duplicated code. As a result, the variables that
store the result could be made `const`.
@avdg81 avdg81 dismissed stale reviews from WPK4FEM and markelov208 via c702f02 May 1, 2024 13:56
@avdg81 avdg81 requested review from rfaasse, markelov208 and WPK4FEM May 1, 2024 13:56
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.

Nicely reduced the duplications, looks good to go to me!

Copy link
Contributor

@WPK4FEM WPK4FEM left a comment

Choose a reason for hiding this comment

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

Looks consistent to me. No comments from my side.

@avdg81 avdg81 enabled auto-merge (squash) May 1, 2024 14:58
@avdg81 avdg81 merged commit 8332197 into master May 1, 2024
11 checks passed
@avdg81 avdg81 deleted the geo/extract-integration-coefficients branch May 1, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GeoMechanics Issues related to the GeoMechanicsApplication Refactor When code is moved or rewrote keeping the same behavior
Projects
None yet
4 participants