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

Added interpolation Lagrange{2,RefCube,3} #701

Closed
wants to merge 0 commits into from

Conversation

JanM12
Copy link
Contributor

@JanM12 JanM12 commented May 10, 2023

introduced interpolation Lagrange{2,RefCube,3} to src/interpolations.jl

Copy link
Member

@koehlerson koehlerson left a comment

Choose a reason for hiding this comment

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

src/interpolations.jl Outdated Show resolved Hide resolved
@fredrikekre fredrikekre changed the title added interpolation Added interpolation Lagrange{2,RefCube,3} May 10, 2023
@fredrikekre
Copy link
Member

fredrikekre commented May 11, 2023

Looks like the tolerance here

@test N_dof[k] 0.0 atol=4eps(Float64) #broken=typeof(interpolation)==Lagrange{2, RefTetrahedron, 5}&&dof==4&&k==18
needs to be adjusted (or possibly the long expressions here can be reordered to give a smaller error like what was done in #633, but maybe not possible).

@termi-official
Copy link
Member

Give me some more time to think about this adjustment, because I am not really sure about what could break downstream due to such spurious evaluations and what the related thresholds to prevent numerical oscillations are. Also I do not know how these affect the overall assembly precision. Generally speaking we should still be able to reorder the expressions to eliminate errors in the tested delta property (even though it is not trivial).

@fredrikekre
Copy link
Member

Perhaps we can BigFloats and truncate to Float64 after evaluation.

@termi-official
Copy link
Member

But this will be a big performance hit for the value call then.

@fredrikekre
Copy link
Member

Maybe, but it is only called in the constructor.

@AbdAlazezAhmed
Copy link
Collaborator

How about using Rational ?

@termi-official
Copy link
Member

Thanks for this suggestions!

Maybe, but it is only called in the constructor.

For order 3 (and maybe order 4) this should be fine, but when moving to higher order one quickyl runs into the issue that assembly degrades solver performance quite severly. Here usually matrix-free techniques are deployed which require reevaluation of the basis functions. I think BigFloat could hurt performance in this scenario quite a bit. To not delay this PW we can go either way and discuss this in a separate issue/discussion, investigating this in more detail (and maybe gathering some literature on this).

How about using Rational ?

This would certainly fix the tests, but IIRC then general quadrature points are unfortunately Irrational. However, I am happy to get corrected if I am wrong on this.

@AbdAlazezAhmed
Copy link
Collaborator

This would certainly fix the tests, but IIRC then general quadrature points are unfortunately Irrational. However, I am happy to get corrected if I am wrong on this.

You're right. AccurateArithmetic.jl can be helpful?

@termi-official
Copy link
Member

Good point. Kahan summation could solve the problem already.

@termi-official
Copy link
Member

Okay maybe we can bring this over the finish line as follows. We can increase the number of points in the partition of unity test here (points outside the domain should be rejected!)

n_basefuncs = getnbasefunctions(interpolation)
x = rand(Tensor{1, ref_dim})
f = (x) -> Ferrite.value(interpolation, Tensor{1, ref_dim}(x))
@test vec(ForwardDiff.jacobian(f, Array(x))')
reinterpret(Float64, Ferrite.derivative(interpolation, x))
@test sum(Ferrite.value(interpolation, x)) 1.0

and increase the tolerance here

@test N_dof[k] 0.0 atol=4eps(Float64) #broken=typeof(interpolation)==Lagrange{2, RefTetrahedron, 5}&&dof==4&&k==18
to 1e-14 . This should be reasonably small for now.

Alternatively we can try to overdub the value calls in the tests with Kahan summations, but this might be a bit more work. It think the linked package fails with AD (just skipped over the implementation).

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.

5 participants