Conversation
…e 3d more consistent with 2D)
…impact as possible
…stitutive dimension is merged to master)
…-soil-model-for-interface-element
|
makes sense to me |
markelov208
left a comment
There was a problem hiding this comment.
Hi Richard, thank you for the clear PR. I have a non-blocking comment.
applications/GeoMechanicsApplication/custom_constitutive/small_strain_umat_law.cpp
Outdated
Show resolved
Hide resolved
avdg81
left a comment
There was a problem hiding this comment.
Hi Richard,
Thanks a lot for generalizing the primary UMAT constitutive law and for adding a proper UMAT constitutive law for two-dimensional line interfaces. I feel the changes are clear, especially with the comments explaining about the wrong size assumptions in most (if not all) of our UMAT libraries.
I have several suggestions, but none of them is blocking in my opinion.
One more thing: can you please rerun Clang-format before merging this branch into master? Thanks.
| /// Pointer definition of SmallStrainUMAT2DInterfaceLaw | ||
| KRATOS_CLASS_POINTER_DEFINITION(SmallStrainUMAT2DInterfaceLaw); | ||
|
|
||
| SmallStrainUMAT2DInterfaceLaw() = default; |
There was a problem hiding this comment.
Just out of curiosity: does the default constructor need to be publicly available? I'm aware that the default constructor is used for deserialization, but since the Serializer class is a friend, it will have access to the private members of the befriended class. How do you see that?
There was a problem hiding this comment.
Not necessary indeed, moved
There was a problem hiding this comment.
I've noticed that the save and load member functions supply the incorrect base class: we should use SmallStrainUMATLaw<VOIGT_SIZE_3D> rather than ConstitutiveLaw.
| /// Pointer definition of SmallStrainUMAT2DPlaneStrainLaw | ||
| KRATOS_CLASS_POINTER_DEFINITION(SmallStrainUMAT2DPlaneStrainLaw); | ||
|
|
||
| SmallStrainUMAT2DPlaneStrainLaw() = default; |
There was a problem hiding this comment.
I've got the same question here about the access specifier of the default constructor: can we make it private rather than public?
| explicit SmallStrainUMAT2DInterfaceLaw(std::unique_ptr<ConstitutiveLawDimension> pConstitutiveDimension) | ||
| : SmallStrainUMATLaw<VOIGT_SIZE_3D>(std::move(pConstitutiveDimension)) | ||
| { | ||
| } |
There was a problem hiding this comment.
I'd rather move the implementation of this constructor to the .cpp file, and only keep the declaration here.
| explicit SmallStrainUMAT2DPlaneStrainLaw(std::unique_ptr<ConstitutiveLawDimension> pConstitutiveDimension) | ||
| : SmallStrainUMATLaw<VOIGT_SIZE_3D>(std::move(pConstitutiveDimension)) | ||
| { | ||
| } |
There was a problem hiding this comment.
Also here I'd rather move the constructor's implementation to the .cpp file.
| rValue.resize(mStressVectorFinalized.size()); | ||
|
|
||
| noalias(rValue) = mStressVectorFinalized; | ||
| if (rValue.size() != TVoigtSize) rValue.resize(TVoigtSize); |
There was a problem hiding this comment.
The size check is also carried out by the resize operation, so we don't need to do it here:
| if (rValue.size() != TVoigtSize) rValue.resize(TVoigtSize); | |
| rValue.resize(TVoigtSize); |
| for (unsigned int i = 0; i < TVoigtSize; ++i) { | ||
| rValue[i] = mStressVectorFinalized[i]; | ||
| } |
There was a problem hiding this comment.
If we can use std::copy_n here, I'd prefer that.
There was a problem hiding this comment.
Reverted to just assignment
| std::vector<Matrix> LineInterfaceElement::CalculateConstitutiveMatricesAtIntegrationPoints( | ||
| const ProcessInfo& rProcessInfo, const std::vector<Vector>& rRelativeDisplacements) |
There was a problem hiding this comment.
As far as I know, we normally have a reference-to-const ProcessInfo instance as the final function parameter. I would suggest to apply that order here as well.
| std::vector<Matrix> CalculateConstitutiveMatricesAtIntegrationPoints(const std::vector<ConstitutiveLaw::Pointer>& rConstitutiveLaws, | ||
| const Properties& rProperties, | ||
| const ProcessInfo& rProcessInfo, | ||
| const std::vector<Vector>& rRelativeDisplacements) |
There was a problem hiding this comment.
As far as I know, we normally have a reference-to-const ProcessInfo instance as the final function parameter. I would suggest to apply that order here as well:
| std::vector<Matrix> CalculateConstitutiveMatricesAtIntegrationPoints(const std::vector<ConstitutiveLaw::Pointer>& rConstitutiveLaws, | |
| const Properties& rProperties, | |
| const ProcessInfo& rProcessInfo, | |
| const std::vector<Vector>& rRelativeDisplacements) | |
| std::vector<Matrix> CalculateConstitutiveMatricesAtIntegrationPoints(const std::vector<ConstitutiveLaw::Pointer>& rConstitutiveLaws, | |
| const Properties& rProperties, | |
| const std::vector<Vector>& rRelativeDisplacements, | |
| const ProcessInfo& rProcessInfo) |
There was a problem hiding this comment.
I've noticed this is a copy of the MaterialParameters.json file used for the multi-stage analysis. Perhaps we can share this file across both analyses?
There was a problem hiding this comment.
I'll wait with this last one until Mohameds integration tests are also in.
rfaasse
left a comment
There was a problem hiding this comment.
Processed review comments
applications/GeoMechanicsApplication/custom_constitutive/small_strain_umat_law.cpp
Outdated
Show resolved
Hide resolved
| /// Pointer definition of SmallStrainUMAT2DInterfaceLaw | ||
| KRATOS_CLASS_POINTER_DEFINITION(SmallStrainUMAT2DInterfaceLaw); | ||
|
|
||
| SmallStrainUMAT2DInterfaceLaw() = default; |
There was a problem hiding this comment.
Not necessary indeed, moved
| explicit SmallStrainUMAT2DInterfaceLaw(std::unique_ptr<ConstitutiveLawDimension> pConstitutiveDimension) | ||
| : SmallStrainUMATLaw<VOIGT_SIZE_3D>(std::move(pConstitutiveDimension)) | ||
| { | ||
| } |
|
|
||
| // We compute the stress | ||
| SmallStrainUMAT3DLaw::CalculateMaterialResponseCauchy(rParameterValues); | ||
| SmallStrainUMATLaw<TVoigtSize>::CalculateMaterialResponseCauchy(rParameterValues); |
| rValue.resize(mStressVectorFinalized.size()); | ||
|
|
||
| noalias(rValue) = mStressVectorFinalized; | ||
| if (rValue.size() != TVoigtSize) rValue.resize(TVoigtSize); |
| * @brief Dimension of the law: | ||
| */ | ||
| SizeType WorkingSpaceDimension() override { return Dimension; } | ||
| SizeType WorkingSpaceDimension() override { return mpConstitutiveDimension->GetDimension(); } |
| std::vector<Matrix> CalculateConstitutiveMatricesAtIntegrationPoints(const std::vector<ConstitutiveLaw::Pointer>& rConstitutiveLaws, | ||
| const Properties& rProperties, | ||
| const ProcessInfo& rProcessInfo, | ||
| const std::vector<Vector>& rRelativeDisplacements) |
| std::vector<Matrix> LineInterfaceElement::CalculateConstitutiveMatricesAtIntegrationPoints( | ||
| const ProcessInfo& rProcessInfo, const std::vector<Vector>& rRelativeDisplacements) |
There was a problem hiding this comment.
I'll wait with this last one until Mohameds integration tests are also in.
…-soil-model-for-interface-element
avdg81
left a comment
There was a problem hiding this comment.
Thank you very much for processing the review suggestions. I have one very minor suggestion, but feel free to not apply it (now). If that's the only thing left, please merge this PR.
| ///@} | ||
|
|
||
| ///@} addtogroup block | ||
| ///@} addtogroup blocksmal |
There was a problem hiding this comment.
Nitpicking: this change was probably not intended. No need to revert it when there are no other things to be changed.
There was a problem hiding this comment.
Whoops, seems like the start of a search command that ended up here. I'll fix it when we add the 3D umat interface version 👍
📝 Description
This PR extends the existing UMAT laws, using a template parameter and
ConstitutiveLawDimensioninstance, such that they can be used with the correct dimensions needed by the new interfaces.Note that the existing functionality is kept identical. It might seem counterintuitive, but the existing 2D/interface UMAT laws need to have stress/strain vectors and a D matrix with sizes consistent with 3D models. This is because most UMATs we encounter, assume these sizes (hard-coded), meaning they will write in out-of-bounds memory if the sizes are smaller.
For our newly registered UMAT law, the sizes are as expected.