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

changed deviator trace assertion #2044

Merged
merged 1 commit into from Feb 20, 2018

Conversation

Projects
None yet
4 participants
@Scinopode
Copy link

Scinopode commented Jan 12, 2018

Used diagonal of tensor and fixed norm

@endJunction
Copy link
Member

endJunction left a comment

Thanks for fixing the norms and using the diagonal is definitely better.

@@ -16,7 +16,7 @@ double Invariants<KelvinVectorSize>::equivalentStress(
Eigen::Matrix<double, KelvinVectorSize, 1> const& deviatoric_v)
{
assert(std::abs(trace(deviatoric_v)) <=
1e-16 * std::abs(deviatoric_v.squaredNorm()));

This comment has been minimized.

@chleh

chleh Jan 12, 2018

Collaborator

deviatoric_v is a column vector, so deviatoric_v.diagonal() is only the first entry of that vector. Is that intended?
Does this small check lead to failing tests/simulations and therefore has to be changed?

This comment has been minimized.

@endJunction

endJunction Jan 14, 2018

Member

Seems like I made a mistake ;)

@endJunction endJunction force-pushed the Scinopode:KelvinVector_Assert branch from 8530d67 to a91d074 Jan 18, 2018

changed deviator trace assertion
Used diagonal of tensor and fixed norm

@endJunction endJunction force-pushed the Scinopode:KelvinVector_Assert branch from a91d074 to c462da9 Feb 16, 2018

@endJunction endJunction added please review and removed WIP 🏗 labels Feb 17, 2018

@endJunction

This comment has been minimized.

Copy link
Member

endJunction commented Feb 17, 2018

Rebased to the master having the Kelvin vectors in the MathLib. Fixed the getting of the diagonal.

@chleh

chleh approved these changes Feb 19, 2018

/// Trace of the corresponding tensor.
template <int KelvinVectorSize>
double Invariants<KelvinVectorSize>::trace(
Eigen::Matrix<double, KelvinVectorSize, 1> const& v)
{
return v.template topLeftCorner<3, 1>().sum();
return diagonal(v).sum();

This comment has been minimized.

@chleh

chleh Feb 19, 2018

Collaborator

The former solution might have been more efficient, because diagonal() creates a temporary.

This comment has been minimized.

@endJunction

endJunction Feb 19, 2018

Member

Good point! I'll check this.

This comment has been minimized.

@endJunction

endJunction Feb 19, 2018

Member

Looks like there is no temporary. I made a small setup

    KelvinVectorType<3> const kv = KelvinVectorType<3>::Random();
    Invariants<6> const invariants;
    volatile double t = invariants.trace(kv);

and changed the implementation of trace btw. topLeftCorner and diagonal. There is no difference in the assembly code (in release mode).

I'd guess that because the diagonal method is inline (being a template), or return value optimization works, or both....

Anyway no difference.

If there are no further comments, I'll merge it tonight.

@endJunction endJunction merged commit 1f91368 into ufz:master Feb 20, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.