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

Higher order gradients in CellValues #938

Merged
merged 17 commits into from
May 29, 2024
Merged

Higher order gradients in CellValues #938

merged 17 commits into from
May 29, 2024

Conversation

lijas
Copy link
Collaborator

@lijas lijas commented May 22, 2024

New version of #891 so that I can see preview of docs. I also needed to rebase on master

Implementation of higher order gradients in CellValues. This was quite easy to do thanks to Knuts new FunctionValues and GeometryMapping.

  • Tests for FaceValues
  • Update changelog
  • Type stable CellValues constructor (?)

@KnutAM
Copy link
Member

KnutAM commented May 22, 2024

Just posting idea from Slack here for a type-stable ValuesUpdateFlag struct

struct ValuesUpdateFlags{FunDiffOrder, GeoDiffOrder, DetJdV} end
function ValuesUpdateFlags(ip_fun; update_gradients, update_hessians, update_detJdV)
    if update_gradients === update_hessians === update_detJdV === nothing
        return ValuesUpdateFlags{1, required_geo_diff_order(mapping_type(ip_fun), 1), true}() # default input till `CellValues`
    else
        FunDiffOrder = update_hessians ? 2 : (update_gradients ? 1 : 0)
        GeoDiffOrder = max(required_geo_diff_order(mapping_type(ip_fun), FunDiffOrder), update_detJdV)
        return ValuesUpdateFlags{FunDiffOrder, GeoDiffOrder, update_detJdV}()
    end
end

Copy link

codecov bot commented May 24, 2024

Codecov Report

Attention: Patch coverage is 97.47899% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 93.69%. Comparing base (5e894e4) to head (7de2176).
Report is 2 commits behind head on master.

Files Patch % Lines
src/FEValues/FunctionValues.jl 94.54% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #938      +/-   ##
==========================================
- Coverage   93.71%   93.69%   -0.03%     
==========================================
  Files          36       36              
  Lines        5313     5438     +125     
==========================================
+ Hits         4979     5095     +116     
- Misses        334      343       +9     

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

Copy link
Member

@KnutAM KnutAM left a comment

Choose a reason for hiding this comment

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

Checked the derivations - nice job! Didn't spot anything being off. Only a few suggestions for a bit clearer formatting.

docs/src/topics/FEValues.md Outdated Show resolved Hide resolved
docs/src/topics/FEValues.md Outdated Show resolved Hide resolved
docs/src/topics/FEValues.md Outdated Show resolved Hide resolved
docs/src/topics/FEValues.md Outdated Show resolved Hide resolved
src/interpolations.jl Outdated Show resolved Hide resolved
Copy link
Member

@KnutAM KnutAM left a comment

Choose a reason for hiding this comment

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

Very nice @lijas.

test/test_utils.jl Outdated Show resolved Hide resolved
test/test_utils.jl Outdated Show resolved Hide resolved
src/FEValues/common_values.jl Outdated Show resolved Hide resolved
src/FEValues/GeometryMapping.jl Show resolved Hide resolved
src/FEValues/GeometryMapping.jl Outdated Show resolved Hide resolved
src/PointEvalHandler.jl Outdated Show resolved Hide resolved
src/PointEvalHandler.jl Outdated Show resolved Hide resolved
src/PointEvalHandler.jl Outdated Show resolved Hide resolved
@KnutAM KnutAM self-requested a review May 28, 2024 15:35
Copy link
Member

@KnutAM KnutAM left a comment

Choose a reason for hiding this comment

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

LGTM after fixing these comments + the CI format/imports complaints 🚀

I suggest documenting this option as experimental simply to be a bit defensive on committing to the keyword interface before we have tested that a bit in practice (since it inevitabily leads to a type-unstable constructor when provided). But perhaps I'm overly careful here?

@@ -1,5 +1,5 @@
"""
CellValues([::Type{T},] quad_rule::QuadratureRule, func_interpol::Interpolation, [geom_interpol::Interpolation])
CellValues([::Type{T},] quad_rule::QuadratureRule, func_interpol::Interpolation, [geom_interpol::Interpolation]; update_detJdV=true, update_gradients=true, update_hessians=false)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CellValues([::Type{T},] quad_rule::QuadratureRule, func_interpol::Interpolation, [geom_interpol::Interpolation]; update_detJdV=true, update_gradients=true, update_hessians=false)
CellValues([::Type{T},] quad_rule::QuadratureRule, func_interpol::Interpolation, [geom_interpol::Interpolation])

src/FEValues/CellValues.jl Outdated Show resolved Hide resolved
src/FEValues/FacetValues.jl Outdated Show resolved Hide resolved
src/FEValues/FacetValues.jl Outdated Show resolved Hide resolved
src/FEValues/FunctionValues.jl Outdated Show resolved Hide resolved
src/FEValues/FunctionValues.jl Outdated Show resolved Hide resolved
end
@test function_value(fv, i, u_vector) ≈ (@test_deprecated function_value(fv, i, u))
function_value(fv, i, ue_vec)
Copy link
Member

Choose a reason for hiding this comment

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

This gives a warning because it is deprecated, why is this teste removed and there is just a function call?

@lijas lijas merged commit cf97ee5 into master May 29, 2024
10 of 11 checks passed
@lijas lijas deleted the eb_difforder2 branch May 29, 2024 13:32
KnutAM added a commit that referenced this pull request Aug 2, 2024
Update devdocs to consider #997, and adds the hessian functions from #938
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.

3 participants