Skip to content

Use AutoDiffScalar Concept and fix checkFESByAutoDiff test #329

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 18 commits into from
Oct 21, 2024
Merged

Conversation

henrij22
Copy link
Collaborator

No description provided.

@henrij22 henrij22 linked an issue Sep 27, 2024 that may be closed by this pull request
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.86%. Comparing base (8e2263e) to head (492863b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #329      +/-   ##
==========================================
- Coverage   86.66%   80.86%   -5.80%     
==========================================
  Files          69       68       -1     
  Lines        2234     2299      +65     
==========================================
- Hits         1936     1859      -77     
- Misses        298      440     +142     
Flag Coverage Δ
tests 80.86% <100.00%> (-5.80%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tarun-mitruka tarun-mitruka changed the title Use AutoDiff Scalar Concept Use AutoDiffScalar Concept Sep 30, 2024
@henrij22 henrij22 requested a review from rath3t September 30, 2024 09:35
@tarun-mitruka tarun-mitruka added bug Something isn't working enhancement New request to enhance existing features labels Sep 30, 2024
@tarun-mitruka
Copy link
Collaborator

tarun-mitruka commented Sep 30, 2024

This action still failed irrespective of the fact that no floating-point exceptions were raised in this commit(note that this failure is not attributed from this PR).
The tests are hence not yet stable and I would recommend to wait before merging this branch and finding a fix before, even though it looks like the CI works at the moment

@rath3t
Copy link
Collaborator

rath3t commented Oct 1, 2024

This action still failed irrespective of the fact that no floating-point exceptions were raised in this commit(note that this failure is not attributed from this PR). The tests are hence not yet stable and I would recommend to wait before merging this branch and finding a fix before, even though it looks like the CI works at the moment

The 3d test does not touch the vanishing stress code or did you find out something different?

The test now runs locally without any exceptions from the floating point?

@tarun-mitruka
Copy link
Collaborator

This action still failed irrespective of the fact that no floating-point exceptions were raised in this commit(note that this failure is not attributed from this PR). The tests are hence not yet stable and I would recommend to wait before merging this branch and finding a fix before, even though it looks like the CI works at the moment

The 3d test does not touch the vanishing stress code or did you find out something different?

No, it doesn't. At least for this commit, all the actions were successful.

@tarun-mitruka tarun-mitruka marked this pull request as draft October 4, 2024 12:30
@tarun-mitruka
Copy link
Collaborator

tarun-mitruka commented Oct 7, 2024

OBSERVATIONS:
Here, I am documenting the current observations:

  • Only executing autoDiffTest<2>(/* ... */) always works (see here)
  • Only executing autoDiffTest<3>(/* ... */) always works (see here)
  • Executive the above two tests simultaneously results in some issues:
  1. autoDiffTest<3>(/* ... */) fails ONLY for clang compiler when using CornerDistortionFlag::randomlyDistorted with nu !=0 and d.setRandom(nDOF)
  2. autoDiffTest<3>(/* ... */) fails for BOTH clang and gcc compilers when using CornerDistortionFlag::unDistorted with nu !=0 and d.setRandom(nDOF) (see here)

The test fails irrespective of the number of minimum iterations (see here) allowed and presence of perturbation the stresses (see here) in vanishingstress.hh

Further debugging is necessary!!

!!! UPDATE
The autodiff<gridDim>(/* ... */) test executed immediately after autoDiffTest<2>(t, planeStressMat2, " nu = 0"); fails, irrespective of gridDim and nu, but only for non-zero displacements. with CornerDistortionFlag::unDistorted.

@rath3t
Copy link
Collaborator

rath3t commented Oct 9, 2024

Since it only fails if the tests are executed simultaneously, it sound still like an uninitialized memory problem or similar.
Therefore, I would suggest executing the test with valgrind to check for issues.

I would suggest, the following procedure, if the 3D-test would fail independently

  1. Extract some configuration (node positions+ displacements) for which the tests fail
  2. Modify the test to explicitly check for the same configuration
  3. Check if NeoHooke and StVenantKirchhoff, become the same for displacements going to zero and Poisson's ratio going to zero
  4. Reduce the formulation to a single integration point
  5. Extract the corresponding strain for which the test fails
  6. Create a new test without any element but only with the material and check if energy,stresses and tangent moduli are correct/the same for autodiff and implementation (This test should already exists somewhere)

@tarun-mitruka tarun-mitruka changed the title Use AutoDiffScalar Concept Use AutoDiffScalar Concept and fix checkFESByAutoDiff test Oct 16, 2024
use FloatCmp and change tolerance

remove noexcept
@tarun-mitruka
Copy link
Collaborator

I think I found the source of the error. I ran the tests 7 times and it always works now (see this action run).

CornerDistortionFlag::randomlyDistorted adds distortions in the range [-0.2, 0.2] to the nodal positions of the reference cube element. (see here).
d.setRandom(basis.flat().dimension()) creates a vector with entities in the range [-1, 1] (from Eigen). This can sometimes leads to a heavily distorted element with a negative deformation gradient.
This breaks the sqrt function here in NeoHooke as it is not defined for negative values.
I think depending on the randomness, the observations I mentioned here was a special case of the above mentioned issue (undefined behavior).
I now scaled the displacements in checkFESByAutoDiff such that the random values in the displacement vectors are in the range [-0.2, 0.2] and also added checks in NeoHooke such that it throws a proper error message when detC < 0. May be this can be scaled down further for extreme case scenarios?

@@ -28,8 +28,8 @@ int main(int argc, char** argv) {
t.subTest(NonLinearElasticityLoadControlNRandTR<Grids::Yasp>(matNH1));
t.subTest(NonLinearElasticityLoadControlNRandTR<Grids::IgaSurfaceIn2D>(matNH1));

autoDiffTest<2>(t, planeStressMat1, " nu != 0");
autoDiffTest<2>(t, planeStressMat2, " nu = 0");
autoDiffTest<2>(t, planeStressMat1, " nu != 0", 1e-7);
Copy link
Collaborator

@tarun-mitruka tarun-mitruka Oct 16, 2024

Choose a reason for hiding this comment

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

This tolerance isn't always necessary but only for certain cases depending on the randomness in the element geometry and the displacement vector (see comment). I am not sure if this is good enough or it could be resolved in a better way? May be fixing the random displacement vector in checkFESByAutoDiff such that this tolerance can be reduced further? We can also fix the tolerance while working on the Issue #211. Do you think increasing minIter to 2 or 3 for NewtonRaphson in VanishingStress would help here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Increasing minIter might help but I'm not sure here. But using tolerances here is not to bad. But I think there is no nice way to fix this. We might refactor isApproxSame which then uses the objects to compute something reasonable: Maybe something like that https://godbolt.org/z/xEPraWKEf

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay then I leave the tolerance here as such and we can refactor isApproxSame while perhaps working on #211

@tarun-mitruka tarun-mitruka marked this pull request as ready for review October 16, 2024 14:18
@tarun-mitruka tarun-mitruka requested a review from rath3t October 16, 2024 14:18
@rath3t
Copy link
Collaborator

rath3t commented Oct 16, 2024

that makes a lot of sense. I will look into it tomorrow.

@tarun-mitruka
Copy link
Collaborator

tarun-mitruka commented Oct 17, 2024

The code coverage is also behaving randomly, I don't know why. Before I committed the change from MathError to InvalidStateException, codecov/project increased by +2.01% and after the commit it decreased with -6.36%. How can this happen?

const auto detC = C.determinant();
if (Dune::FloatCmp::lt(static_cast<double>(detC), 0.0, 1e-10))
DUNE_THROW(Dune::InvalidStateException,
"Determinant of right Cauchy Green tensor C must be greater than or equal to zero. detC = " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be equal to zero. It should really be bigger. From theory.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You mean it shouldn't be equal to zero? So change FloatCmp::lt to FloatCmp::le?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah sorry. I missed a word. It should not be equal to zero. Therefore detC>0. The statement in the if statement is fine but the exception text is saying detC>=0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I fixed it

@@ -96,7 +101,12 @@ struct NeoHookeT : public Material<NeoHookeT<ST>>
static_assert(Concepts::EigenMatrixOrVoigtNotation3<Derived>);
if constexpr (!voigt) {
if constexpr (!Concepts::EigenVector<Derived>) {
const auto logdetF = log(sqrt(C.determinant()));
const auto detC = C.determinant();
if (Dune::FloatCmp::lt(static_cast<double>(detC), 0.0, 1e-10))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you are repeating that functionality I would move it to a private member function and call it check positive determinant of C.

if constexpr (std::is_same_v<ScalarType, double>)
return mat_.template tangentModuli<strainType, voigt>(strain);
else {
if constexpr (Concepts::AutodiffScalar<ScalarType>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we also want a private function getmaterial, which does this compile time if statement and returns the correct material object. Make sure that decltype(auto) is placed everywhere to not introduce copies


t.checkThrow<Dune::InvalidStateException>(
[&]() {
const auto moduli = (planeStress(matNH, 1e-8).template tangentModuli<StrainTags::greenLagrangian, true>(E));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe not repeat planeStress(matNH, 1e-8) but use the same object all the time?

@rath3t
Copy link
Collaborator

rath3t commented Oct 21, 2024

The code coverage is also behaving randomly, I don't know why. Before I committed the change from MathError to InvalidStateException, codecov/project increased by +2.01% and after the commit it decreased with -6.36%. How can this happen?

This is also a mystery for me, therefore I take these changes always with a grain of salt. We might remove that even or change to another provider

@rath3t
Copy link
Collaborator

rath3t commented Oct 21, 2024

In total for me this looks quite fine and the comments might also happen in another PR except for the code duplication stuff. Still you can quickly look into if you have an idea that can be done fast.
Thanks Tarun for investigation!

@tarun-mitruka tarun-mitruka requested a review from rath3t October 21, 2024 12:36
Copy link
Collaborator

@rath3t rath3t left a comment

Choose a reason for hiding this comment

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

Ok this is fine for me.

@rath3t rath3t merged commit 2ff5c77 into main Oct 21, 2024
21 of 22 checks passed
@rath3t rath3t deleted the fix/adconcept branch October 21, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New request to enhance existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

concept AutodiffScalar has to be used wherever applicable
3 participants