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

Convenience constructors CellValues and FaceValues #806

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

KnutAM
Copy link
Member

@KnutAM KnutAM commented Oct 2, 2023

(Taken out and extended from #680)

With the new distinction between vector and scalar interpolations since #694,
it is possible to make a convenience constructor of cell and face values, given a
SubDofHandler (or DofHandler with a single SubDofHandler).

Happy to hear feedback on this idea!

Proposed syntax

cv = CellValues(dh, fieldname; qr)
fv = FaceValues(dh, fieldname; qr)

where qr::Union{Int,[Face]QuadratureRule}. For standard quadrature rules, the syntax now allows e.g.,

cv = CellValues(dh, :u; qr=2) # Equivalent to `CellValues(QuadratureRule{RefShape}(2), ip, ip_geo)`
fv = FaceValues(dh, :u, qr=1) # Equivalent to `FaceValues(FaceQuadratureRule{RefShape}(2), ip, ip_geo)`

where ip is the interpolation in dh for the field u,
and ip_geo is the default geometric interpolation for the cell type in dh.grid.
Using dh::DofHandler is only allowed for single subdofhandler problems,
use dh::SubDofHandler for cases with multiple subdofhandlers.

Motivation
The geometric interpolation matches the grid, without having to extract information by using e.g. Ferrite.default_interpolation on the user-side
Less verbose by not requiring the quadrature rule constructor when the standard quadrature points should be used.

@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d0f0e07) 92.98% compared to head (6f96ea1) 93.04%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #806      +/-   ##
==========================================
+ Coverage   92.98%   93.04%   +0.05%     
==========================================
  Files          33       34       +1     
  Lines        4950     4972      +22     
==========================================
+ Hits         4603     4626      +23     
+ Misses        347      346       -1     

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

@termi-official
Copy link
Member

I absolutely like this! I very often run into the scenario where I have to manage deducing the correct geometric interpolation and it can easily lead to errors - especially for embedded elements! While I also think that Ferrite should be as low level as possible, I also think that this is now the correct level for the user-facing API.

I am just indecisive if we should provide the dispatch with the quadrature order (instead of just using the quadrature rule). Maybe it is also the design of the dispatches which makes this weird. Maybe the quadrature order dispatch should be

CellValues(dh, fieldname, qr_order)
CellValues(dh, fieldname, qr_order, qr_symbol)

But then I am also not sure about qr_symbol, because it changes between refshape types.

@termi-official
Copy link
Member

termi-official commented Nov 11, 2023

OT: Also, if we already think about convenience functions, what about dispatches for interpolations and quadrature rules taking in the grid or grid+cellset as input argument to deduce the ref shape type?

@KnutAM
Copy link
Member Author

KnutAM commented Nov 12, 2023

I absolutely like this!

Happy to hear!

I am just indecisive if we should provide the dispatch with the quadrature order

I see the point that this is the only thing that is not fully inferrable from the input (i.e. the symbol). But since in practice all examples use the standard QuadratureRule{RefShape}(order) call, I thought this was a reasonable default, and if using a non-standard type (which, as you say depends on the refshape), I think it makes sense to pass the full quadrature rule.

I think using keyword argument is better than positional argument for this case, as I think CellValues(dh, :u; qr=1) is more readable than CellValues(dh, :u, 1)

Also, if we already think about convenience functions, what about dispatches for interpolations and quadrature rules taking in the grid or grid+cellset as input argument to deduce the ref shape type?

Interesting to explore possibilities for that too! I left a separate issue, #841 with my suggestion!

Copy link
Member

@termi-official termi-official left a comment

Choose a reason for hiding this comment

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

LGTM.

@termi-official termi-official added the awaiting review PR is finished from the authors POV, waiting for feedback label Nov 15, 2023
@KnutAM KnutAM mentioned this pull request Jan 11, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review PR is finished from the authors POV, waiting for feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants