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

CellMultiValues (new attempt) #872

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

CellMultiValues (new attempt) #872

wants to merge 23 commits into from

Conversation

KnutAM
Copy link
Member

@KnutAM KnutAM commented Jan 11, 2024

Supersedes #680 and #674, using the components from #764

  • Check performance
  • Update incompressible elasticity example
  • Add tests
  • Complete documentation

A couple of notes on the design

  • Given a cv::CellMultiValues, cv[:u]::FunctionValues, which can be used just like a CellValues for all function-related queries, but not geometrical such as getdetJdV, these are called on cv itself.
  • The constructor accepts a named tuple of function interpolations. If two interpolations are identical, they will share the same FunctionValues object, saving both memory (bandwidth) and computations in reinit!.
  • FunctionValues subtypes AbstractValues to make all function_value and similar work. This implies that e.g. spatial_coordinate(cv[:u]::FunctionValues, args...) will give a method error on getngeobasefunctions instead of spatial_coordinate. We could add specializations if easier error messages are required.

It would be very convenient to also store the dof_range of each field, but having this as an input would be error-prone if the user provides the wrong order compared to what is stored in the SubDofHandler. If a convenience constructor similar to #806 would be used, this would be safe, but that should only be a high-level constructor interface. Ideas are welcome! (Edit: not generally applicable, so leave out for now)

@KnutAM KnutAM changed the title MultiCellValues (new attempt) CellMultiValues (new attempt) Feb 24, 2024
Copy link

codecov bot commented Feb 24, 2024

Codecov Report

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

Project coverage is 93.33%. Comparing base (58ca229) to head (8016860).

Files Patch % Lines
src/FEValues/CellValues.jl 98.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #872      +/-   ##
==========================================
+ Coverage   93.30%   93.33%   +0.02%     
==========================================
  Files          36       36              
  Lines        5257     5308      +51     
==========================================
+ Hits         4905     4954      +49     
- Misses        352      354       +2     

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

@KnutAM KnutAM marked this pull request as ready for review February 24, 2024 15:56
@KnutAM
Copy link
Member Author

KnutAM commented Feb 24, 2024

I've renamed to CellMultiValues following @termi-official's suggestion. I find this name harder when coding, but that could be me having become accustomed to MultiCellValues.
Another option could be CellValuesGroup

Otherwise, ready for review from my side if anyone wants to take a look!

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.

I left some comments.

Since I am struggling recently with the question: Do you think this should be still Ferrite core or in some package on top?

src/FEValues/CellValues.jl Outdated Show resolved Hide resolved
src/FEValues/CellValues.jl Outdated Show resolved Hide resolved
src/FEValues/CellValues.jl Outdated Show resolved Hide resolved
@KnutAM
Copy link
Member Author

KnutAM commented Feb 26, 2024

Do you think this should be still Ferrite core or in some package on top?

I think this belongs in Ferrite core for two main reasons:

  1. Using multiple fields is common, and this feature makes that efficient and easier.
  2. Implementing this without heavily relying on Ferrite internals is hard for an external package.

But of course, this is my biased opinion :)

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.

2 participants