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.

Sign up for GitHub

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

Jump to bottom

Geo/umat parameters python #11476

Merged
merged 15 commits into from
Oct 12, 2023
Merged

Geo/umat parameters python #11476

merged 15 commits into from
Oct 12, 2023

Conversation

EleniSmyrniou
Copy link
Contributor

Added UMAT_PARAMETERS as a python variable. This is almost always essential when setting up random field so it is a nice minor improvement

@EleniSmyrniou EleniSmyrniou added Python Minor GeoMechanics Issues related to the GeoMechanicsApplication labels Aug 14, 2023
@mcgicjn2
Copy link
Contributor

Why is this "almost always essential" ? It's a random field anything can be random in a material property

@aronnoordam aronnoordam self-requested a review August 14, 2023 14:06
aronnoordam
aronnoordam requested changes Aug 14, 2023
View reviewed changes
Copy link
Member

@aronnoordam aronnoordam left a comment

Choose a reason for hiding this comment

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

placeholder, because I don't remember why umat parameters were required to be exposed to python :D

@mcgicjn2
Copy link
Contributor

Yes the why is important for instance if a c/phi reduction is being created on the python side, we are already developing that officially

@EleniSmyrniou
Copy link
Contributor Author

EleniSmyrniou commented Aug 15, 2023 •
edited
Loading

This goes a bit further than the c/phi reduction and the Mohr-Coulomb model. The problem is if you want to create a random field based on the parameters of a UMAT model you need to be able to change the UMAT_PARAMETERS. That is why it is almost essential for creating RFs with UMATs, since almost all the parameters you would like to change are part of the UMAT_PARAMETERS

@EleniSmyrniou
Copy link
Contributor Author

placeholder, because I don't remember why umat parameters were required to be exposed to python :D

They need to be exposed because to vary the parameters when using a UMAT model, the parameters provided so far in KratosGeo do not suffice.

@EleniSmyrniou EleniSmyrniou reopened this Aug 15, 2023
@EleniSmyrniou EleniSmyrniou self-assigned this Aug 15, 2023
@aronnoordam
Copy link
Member

aronnoordam commented Aug 16, 2023 •
edited
Loading

I think that the set_parameter_field needs to be updated by also accepting vector variables. Currently only double variables can be altered via set_parameter_field. If this is updated, python does not need to know about UMAT variables, since all required data is defined in projectparameters.json

aronnoordam
aronnoordam reviewed Aug 23, 2023
View reviewed changes
Copy link
Member

@aronnoordam aronnoordam left a comment

Choose a reason for hiding this comment

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

nice, few comments and questions

applications/GeoMechanicsApplication/custom_processes/set_parameter_field_process.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_processes/set_parameter_field_process.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/python_scripts/set_parameter_field_process.py Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/test_parameter_field.py Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_processes/set_parameter_field_process.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_processes/set_parameter_field_process.cpp Outdated Show resolved Hide resolved
...tion/tests/test_parameter_field/parameter_field_input_umat_parameters/ProjectParameters.json Outdated Show resolved Hide resolved
...ation/tests/test_parameter_field/parameter_field_json_umat_parameters/ProjectParameters.json Outdated Show resolved Hide resolved
...ion/tests/test_parameter_field/parameter_field_python_umat_parameters/ProjectParameters.json Outdated Show resolved Hide resolved
mcgicjn2
mcgicjn2 requested changes Aug 23, 2023
View reviewed changes
Copy link
Contributor

@mcgicjn2 mcgicjn2 left a comment

Choose a reason for hiding this comment

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

Can you make sure that this change doesn't limit the functionality to only UMAT parameters. This should be optional. Checking that UMAT is present means that we limit this change to only those materials, and thus we cannot for instance do this with any of the kratos coded materials parameters (e.g. linear elastic). This should be generic.

@EleniSmyrniou
Copy link
Contributor Author

Can you make sure that this change doesn't limit the functionality to only UMAT parameters. This should be optional. Checking that UMAT is present means that we limit this change to only those materials, and thus we cannot for instance do this with any of the kratos coded materials parameters (e.g. linear elastic). This should be generic.

This is arleady the case. All parameters defined in Kratos coded materials ( for example YOUNG_MODULUS) can be used as a input to a field. This issue is merely an extension to include the UMAT_PARAMETERS as this variable is a Vector and was not covered by the current code. Functionality of how Kratos handles the UMATS or internal umats was not changed

@aronnoordam
Copy link
Member

aronnoordam commented Aug 23, 2023 •
edited
Loading

Can you make sure that this change doesn't limit the functionality to only UMAT parameters. This should be optional. Checking that UMAT is present means that we limit this change to only those materials, and thus we cannot for instance do this with any of the kratos coded materials parameters (e.g. linear elastic). This should be generic.

This is arleady the case. All parameters defined in Kratos coded materials ( for example YOUNG_MODULUS) can be used as a input to a field. This issue is merely an extension to include the UMAT_PARAMETERS as this variable is a Vector and was not covered by the current code. Functionality of how Kratos handles the UMATS or internal umats was not changed

Ideally it should work for all vector variables, if there is a way to check if variable is of type vector rather than to check for UMAT, that would be ideal. My first comment of today refers to this

@EleniSmyrniou
Copy link
Contributor Author

Can you make sure that this change doesn't limit the functionality to only UMAT parameters. This should be optional. Checking that UMAT is present means that we limit this change to only those materials, and thus we cannot for instance do this with any of the kratos coded materials parameters (e.g. linear elastic). This should be generic.

This is arleady the case. All parameters defined in Kratos coded materials ( for example YOUNG_MODULUS) can be used as a input to a field. This issue is merely an extension to include the UMAT_PARAMETERS as this variable is a Vector and was not covered by the current code. Functionality of how Kratos handles the UMATS or internal umats was not changed

Ideally it should work for all vector variables, if there is a way to check if variable is of type vector rather than to check for UMAT, that would be ideal. My first comment of today refers to this

Indeed if anyone has an idea on how to check from a string the type of variable in Kratos, I will be happy to include it

avdg81 and others added 3 commits August 28, 2023 11:25
@avdg81
Several code improvements
21c92cc 
- Removed a few duplicated member functions by introducing member
  function templates.
- The `SetParameterFieldProcess` should now be applicable to any
  `double` variable or `Vector` variable.
- Renamed a few variables.
- Removed a few redundant blank lines and some redundant comments.
- Rewrote a few `for` loops in a more compact way.
- Changed two function parameters to reference-to-const. As a result, we
  can now also pass temporary objects.
- Added a function to get the vector indices. This function also takes
  care of converting the index to the correct type (`double` =>
  `IndexType`).
Renamed variable
e215292
Corrected according to reviewer's comments
05a3c5d
@avdg81 avdg81 requested a review from rfaasse September 27, 2023 07:01
rfaasse
rfaasse approved these changes Sep 27, 2023
View reviewed changes
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.

The code looks very clean and clear (as a new joiner for the Kratos GeoMechanics team, I learned quite a bit about the UMAT process during this review)!

I only have one minor suggestion, but that isn't blocking (it's close to a general personal preference, so feel free to implement or ignore it).

applications/GeoMechanicsApplication/custom_elements/U_Pw_small_strain_element.cpp Show resolved Hide resolved
@EleniSmyrniou EleniSmyrniou removed the request for review from aronnoordam September 27, 2023 14:47
@EleniSmyrniou EleniSmyrniou merged commit 291c05f into master Oct 12, 2023
@EleniSmyrniou EleniSmyrniou deleted the geo/umat_parameters_python branch October 12, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rfaasse rfaasse rfaasse approved these changes

@aronnoordam aronnoordam aronnoordam approved these changes

@avdg81 avdg81 avdg81 approved these changes

@mcgicjn2 mcgicjn2 mcgicjn2 approved these changes

Assignees

@EleniSmyrniou EleniSmyrniou

Labels
GeoMechanics Issues related to the GeoMechanicsApplication Minor Python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@EleniSmyrniou @mcgicjn2 @aronnoordam @avdg81 @rfaasse