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

Geo/12646 pop for k0 procedure process #12651

Merged
merged 67 commits into from
Oct 11, 2024

Conversation

WPK4FEM
Copy link
Contributor

@WPK4FEM WPK4FEM commented Aug 28, 2024

📝 Description
Implementation of POP, documentation and unit testing of K0 procedure process.

🆕 Changelog

  • Added first setup of K0 README.md
  • Integration test for OCR without parameter field
  • Integration test for K0 with OCR
  • definition of POP and first implementation of POP in K0 procedure process
  • comparative tests for POP added
  • unit tests added for K0 procedure process
  • check function for input sanity check added

Copy link
Contributor

@indigocoral indigocoral left a comment

Choose a reason for hiding this comment

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

Thank you for all the work you have done on this. I have a couple of remarks which is more about the style. I'll discuss the questions I have with you later.


# compare cauchy_stress_xx = k0 * cauchy_stress_yy, cauchy_stress_xy = 0.
k0_nc = 0.6
# POP 0.4 * sig'_yy_bottom
Copy link
Contributor

@indigocoral indigocoral Aug 29, 2024

Choose a reason for hiding this comment

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

It is a bit unclear where 0.4 has come from. I think it needs more elaboration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried, please check the revised text.

Copy link
Contributor

@indigocoral indigocoral left a comment

Choose a reason for hiding this comment

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

Hi again :)
Thanks for the explanations to my questions. I've added some more comments (which we already discussed yesterday).

k0_vector *= rProp[OCR];
array_1d<double, 3> correction(3, (PoissonUR / (1.0 - PoissonUR)) * (rProp[OCR] - 1.0));
k0_vector -= correction;
} else if (rProp.Has(POP)) {
POP_value = -rProp[POP];
Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed, adding a comment would be helpful in understanding this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried, please read the added comment.

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.

Thank you for adding this functionality, next to minor comments (typo's etc), there are a few topics I think we should discuss:

  • How do we want to deal with the plx results? I'm not sure if adding these plx.res files and comparing all integration points is the right way to go.
  • A related point is, do we want to be explicit with mentioning plaxis in this open source package? To me it seems risky.
  • There is an extensive integration test suite, but I would be very much in favor of also adding unit tests.
  • The integration tests become easier to review if there's a schematic to go with it (since the meshes are quite large and not easy to visualize)

@WPK4FEM WPK4FEM force-pushed the geo/12646_pop_for_k0_procedure_process branch from 0ad4931 to ea3a2e7 Compare October 2, 2024 15:10
indigocoral
indigocoral previously approved these changes Oct 10, 2024
Copy link
Contributor

@indigocoral indigocoral left a comment

Choose a reason for hiding this comment

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

Wijtze Pieter,
Thank you very much for your work on this. It is very valuable to get POP in K0 procedure process. I have very few minor suggestions on the documentation. And I have also asked my questions to you so all is clear to me. I did not get the chance to review the code unfortunately but I will push my comments anyway. Thank you!

Copy link
Contributor

@avdg81 avdg81 left a comment

Choose a reason for hiding this comment

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

This looks like a clean and well-tested PR to me. I have several suggestions and questions, but nothing major. To all contributors: well done!

@WPK4FEM WPK4FEM marked this pull request as ready for review October 11, 2024 09:02
Copy link
Contributor

@avdg81 avdg81 left a comment

Choose a reason for hiding this comment

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

Thank you very much to all contributors for the time and effort that you have put into this. I believe this PR is ready to be merged.

@WPK4FEM WPK4FEM merged commit dfc8f8e into master Oct 11, 2024
9 of 11 checks passed
@WPK4FEM WPK4FEM deleted the geo/12646_pop_for_k0_procedure_process branch October 11, 2024 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GeoMechanicsApplication] Preparation for applying the POP in the K0 procedure process
5 participants