Skip to content

[Structural] Use Gauss rule in solid shell element #13421

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

Merged
merged 11 commits into from
May 21, 2025

Conversation

rubenzorrilla
Copy link
Member

📝 Description
In line to what has been discussed in #13085, this PR targets arranging the KratosCore integration rules. Specifically, this PR removes the unique use of the problematic extended Gauss rules, that is from the solid shell element.

@loumalouomega I substituted the extended Gauss by the standard ones. Once we have the Lobatto ones properly implemented, we can update it to these if you consider it more appropriate.

PD: el put* elemento maligno ataca de nuevo.

Copy link
Member

@loumalouomega loumalouomega left a comment

Choose a reason for hiding this comment

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

You cannot simply use standard gauss points. The model has specific integration and can be manually replaced, I will do, but please don't use the standard ones

@rubenzorrilla
Copy link
Member Author

You cannot simply use standard gauss points. The model has specific integration and can be manually replaced, I will do, but please don't use the standard ones

Sorry, I wasn't aware of this. Feel free to pirate the branch and the PR.

@loumalouomega
Copy link
Member

I just checked and this is more difficult I remembered... There are two options, or I harcode the shape functions and gradients in the element or I generate a new prism deriving from the base one changing the integration methods.

@rubenzorrilla
Copy link
Member Author

I just checked and this is more difficult I remembered... There are two options, or I harcode the shape functions and gradients in the element or I generate a new prism deriving from the base one changing the integration methods.

Taking into account that seems to be something very specific, I'd go for option one. I also note that this would be much easier too.

@loumalouomega
Copy link
Member

loumalouomega commented May 13, 2025 via email

@rubenzorrilla
Copy link
Member Author

The easier and nastier would be copy paste the geometry :P, but the KISS would be to hardcode the gradients and jacobians. It will take some time, I hope there is no hurry. El mar., 13 may. 2025 7:48, Rubén Zorrilla @.> escribió:

rubenzorrilla left a comment (KratosMultiphysics/Kratos#13421) <#13421 (comment)> I just checked and this is more difficult I remembered... There are two options, or I harcode the shape functions and gradients in the element or I generate a new prism deriving from the base one changing the integration methods. Taking into account that seems to be something very specific, I'd go for option one. I also note that this would be much easier too. — Reply to this email directly, view it on GitHub <#13421 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEYQZAH5NCK33TWNZF43ODD26GBURAVCNFSM6AAAAAB46O5FDKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNZVGEZDCMJTHE . You are receiving this because you were mentioned.Message ID: @.
>

There's no hurry at all. However, I note that a student of us (@AlejandroCornejo) needs this to be finally fixed to advance with his thesis so we might eventually require to flag the SolidShell tests as expected fail and move forward.

@loumalouomega
Copy link
Member

The easier and nastier would be copy paste the geometry :P, but the KISS would be to hardcode the gradients and jacobians. It will take some time, I hope there is no hurry. El mar., 13 may. 2025 7:48, Rubén Zorrilla @.> escribió:

rubenzorrilla left a comment (KratosMultiphysics/Kratos#13421) <#13421 (comment)> I just checked and this is more difficult I remembered... There are two options, or I harcode the shape functions and gradients in the element or I generate a new prism deriving from the base one changing the integration methods. Taking into account that seems to be something very specific, I'd go for option one. I also note that this would be much easier too. — Reply to this email directly, view it on GitHub <#13421 (comment)>, or unsubscribe github.com/notifications/unsubscribe-auth/AEYQZAH5NCK33TWNZF43ODD26GBURAVCNFSM6AAAAAB46O5FDKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNZVGEZDCMJTHE . You are receiving this because you were mentioned.Message ID: _
@**
.**_>

There's no hurry at all. However, I note that a student of us (@AlejandroCornejo) needs this to be finally fixed to advance with his thesis so we might eventually require to flag the SolidShell tests as expected fail and move forward.

It would take me a couple of days, not weeks or months. I would not deactivate tests. BTW, you, me and @AlejandroCornejo we have done our thesis with branches. How can be so important to merge this?, the integrations schemes are basically enums...

@rubenzorrilla
Copy link
Member Author

rubenzorrilla commented May 13, 2025

The easier and nastier would be copy paste the geometry :P, but the KISS would be to hardcode the gradients and jacobians. It will take some time, I hope there is no hurry. El mar., 13 may. 2025 7:48, Rubén Zorrilla @.> escribió:

rubenzorrilla left a comment (KratosMultiphysics/Kratos#13421) <#13421 (comment)> I just checked and this is more difficult I remembered... There are two options, or I harcode the shape functions and gradients in the element or I generate a new prism deriving from the base one changing the integration methods. Taking into account that seems to be something very specific, I'd go for option one. I also note that this would be much easier too. — Reply to this email directly, view it on GitHub <#13421 (comment)>, or unsubscribe github.com/notifications/unsubscribe-auth/AEYQZAH5NCK33TWNZF43ODD26GBURAVCNFSM6AAAAAB46O5FDKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNZVGEZDCMJTHE . You are receiving this because you were mentioned.Message ID: _
@**
.**_>

There's no hurry at all. However, I note that a student of us (@AlejandroCornejo) needs this to be finally fixed to advance with his thesis so we might eventually require to flag the SolidShell tests as expected fail and move forward.

It would take me a couple of days, not weeks or months. I would not deactivate tests. BTW, you, me and @AlejandroCornejo we have done our thesis with branches. How can be so important to merge this?, the integrations schemes are basically enums...

She is an undergraduate student who works with releases. I'd like to avoid creating a release just for her... Nevertheless, if you manage to do it in a couple of days there shouldn't be any problem.

Thanks for taking care of it.

…larations and enhance documentation

- Replaced typedefs with using declarations for improved readability and modern C++ style.
- Enhanced documentation for constructors, methods, and member variables to clarify functionality and usage.
- Corrected typos in comments and improved descriptions for enums and methods.
- Added static methods for integration points and shape functions to streamline calculations.
loumalouomega
loumalouomega previously approved these changes May 19, 2025
@loumalouomega loumalouomega enabled auto-merge May 19, 2025 19:07
@loumalouomega
Copy link
Member

Should be OK now.

loumalouomega
loumalouomega previously approved these changes May 19, 2025
Copy link
Contributor

@matekelemen matekelemen left a comment

Choose a reason for hiding this comment

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

@loumalouomega you shouldn't approve a PR you contributed to.

Also, please do an actual review on something you approve.

@loumalouomega loumalouomega requested a review from a team as a code owner May 20, 2025 10:35
@loumalouomega loumalouomega requested a review from matekelemen May 20, 2025 13:26
…nsistency

- Removed unused IntegrationMethod type alias.
- Reformatted constructor signatures for better readability.
- Updated parameter names for consistency (e.g., `ThisNodes` to `rNodes`).
- Added a new method to return the integration method used.
- Improved documentation comments for clarity.
- Corrected typos in comments and variable names.
- Enhanced code structure by grouping related methods and variables.
@rubenzorrilla
Copy link
Member Author

@matekelemen this is IMO ready to be merged. The changes amend all the current inconveniences of the solid-shell element.

@matekelemen
Copy link
Contributor

I'm on holiday till the end of next week, so feel free to override my block.

@loumalouomega
Copy link
Member

I'm on holiday till the end of next week, so feel free to override my block.

I think it is you who must override it

@rubenzorrilla
Copy link
Member Author

I'm on holiday till the end of next week, so feel free to override my block.

Yes. We need you to approve it.

@matekelemen
Copy link
Contributor

matekelemen commented May 21, 2025

I thought the TC could ignore other collaborators' blocks. Fine, I'll take a look in the next 2 days, though I might not be the expert here. Maybe @AlejandroCornejo or @sunethwarna would be better qualified.

@rubenzorrilla
Copy link
Member Author

I thought the TC could ignore other collaborators' blocks. Fine, I'll take a look in the next 2 days, though I might not be the expert here. Maybe @AlejandroCornejo or @sunethwarna would be better qualified.

@loumalouomega is the expert here. It's a very specific element with lot of particularities inherent to the formulation that he developed during his PhD.

From the @KratosMultiphysics/technical-committee we will acknowledge you unblocking this so we can proceed with #13085. Note that the changes in here do not affect anything aside of the sprism element. Also note that the changes in the geometries are those required after our decision on the quadratures issue (also included by myself in #13085).

Copy link
Contributor

@matekelemen matekelemen left a comment

Choose a reason for hiding this comment

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

Fine.

I'll just stress my dislike for, and the dangers of, self-approved changes ending up so easily in master.

@loumalouomega loumalouomega merged commit 88b55a3 into master May 21, 2025
11 checks passed
@loumalouomega loumalouomega deleted the structural/gauss-rule-in-solid-shell branch May 21, 2025 11:54
@loumalouomega
Copy link
Member

Fine.

I'll just stress my dislike for, and the dangers of, self-approved changes ending up so easily in master.

As @rubenzorrilla said this element is super specific and developed in the context of my thesis, and these changes are part of a bigger PR and this PR was blocking it. I am not doing this sistematically.

@rubenzorrilla
Copy link
Member Author

Fine.

I'll just stress my dislike for, and the dangers of, self-approved changes ending up so easily in master.

No worries. Yours is a very fair point. However, I think we can consider this a very specific case. Also note that the original implementation was mines but we required @loumalouomega to step in because the aforementioned reasons.

Thanks for the quick out-of-office approval.

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.

3 participants