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

(Performance) improvements on gathering field information from MixedDofHandler #621

Merged
merged 14 commits into from
Mar 21, 2023

Conversation

kimauth
Copy link
Member

@kimauth kimauth commented Mar 19, 2023

This PR tackles the functionality that requests field information from the MixedDofHandler, in particular

  • find_field
  • getfielddim
  • getfieldinterpolation
  • field_offset
  • dof_range

Performance improvements

Finding fields based on the field name was a bit suboptimal. From the side of the user API, in particular dof_range profits, but most improvements are based on a better find_field:

#master #PR
@btime dof_range($fh, $(:f3)) 567.935 ns (14 allocations: 672 bytes) 36.715 ns (0 allocations: 0 bytes)
@btime Ferrite.find_field($mixed_dh, $(:f3)) 31.448 ns (1 allocation: 80 bytes) 4.583 ns (0 allocations: 0 bytes)
@btime Ferrite.find_field($fh, $(:f3)) 29.104 ns (1 allocation: 80 bytes) 4.583 ns (0 allocations: 0 bytes)
Benchmark code
using Ferrite
using BenchmarkTools

grid = generate_grid(Quadrilateral, (10, 10))

# three fields, each on all cells
dh = DofHandler(grid)
add!(dh, :f1, 1, Lagrange{2,RefCube,1}())
add!(dh, :f2, 2, Lagrange{2,RefCube,1}())
add!(dh, :f3, 1, Lagrange{2,RefCube,2}())
close!(dh)

mixed_dh = MixedDofHandler(grid)
add!(mixed_dh, :f1, 1, Lagrange{2,RefCube,1}())
add!(mixed_dh, :f2, 2, Lagrange{2,RefCube,1}())
add!(mixed_dh, :f3, 1, Lagrange{2,RefCube,2}())
close!(mixed_dh)

fh = mixed_dh.fieldhandlers[1]

@btime dof_range($fh, $(:f3))

@btime Ferrite.find_field($mixed_dh, $(:f3))
@btime Ferrite.find_field($fh, $(:f3))

Breaking change to find_field(mixed_dh, fieldname)

# master:
julia> Ferrite.find_field(mixed_dh, :f3)
3

# PR
julia> Ferrite.find_field(mixed_dh, :f3)
(1, 3)

find_field now searches for the first occurence of the given field and returns a tuple with the index of the fieldhandler it was found in and the index of the field in the fieldhandler (before it only searched in the first fieldhandler).

API

It is most efficient to call getfielddim, getfieldinterpolation, field_offset and dof_range with field_idxs instead of fieldname as argument. However, in order to have the exact same syntax for MixedDofHandler and DofHandler (working towards merging them), I suggest to have both options for all functions, i.e. possible ways to call e.g. dof_range are:

  • dof_range(mixed_dh, field_idxs)
  • dof_range(mixed_dh, fieldname)
  • dof_range(fh, field_idxs)
  • dof_range(fh, fieldname)

@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2023

Codecov Report

Patch coverage: 66.66% and project coverage change: -0.22 ⚠️

Comparison is base (1c01761) 92.65% compared to head (e9087f7) 92.44%.

❗ Current head e9087f7 differs from pull request most recent head c0ee822. Consider uploading reports for the commit c0ee822 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #621      +/-   ##
==========================================
- Coverage   92.65%   92.44%   -0.22%     
==========================================
  Files          29       29              
  Lines        4441     4463      +22     
==========================================
+ Hits         4115     4126      +11     
- Misses        326      337      +11     
Impacted Files Coverage Δ
src/Dofs/MixedDofHandler.jl 81.70% <66.66%> (-3.12%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@termi-official
Copy link
Member

Thanks for taking the time Kim! Note that find_field is still broken.

using Ferrite

function get_2d_grid()
    # GIVEN: two cells, a quad and a triangle sharing one face
    cells = [
        Quadrilateral((1, 2, 3, 4)),
        Triangle((3, 2, 5))
        ]
    coords = zeros(Vec{2,Float64}, 5)
    nodes = [Node(coord) for coord in zeros(Vec{2,Float64}, 5)]
    return Grid(cells,nodes)
end

function create_field(;name, field_dim, order, spatial_dim, cellshape)
    interpolation = Lagrange{spatial_dim, cellshape, order}()
    return Field(name, interpolation, field_dim)
end

grid = get_2d_grid()
# WHEN: adding a scalar field for each cell and generating dofs
field1 = create_field(name=:u, spatial_dim=2, field_dim=1, order=1, cellshape=RefCube)
field2 = create_field(name=:u, spatial_dim=2, field_dim=1, order=1, cellshape=RefTetrahedron)
dh = MixedDofHandler(grid);
add!(dh, FieldHandler([field1], Set(1)));
add!(dh, FieldHandler([field2], Set(2)));
close!(dh)

Ferrite.find_field(dh, :u)

returns only (1,1).

@kimauth
Copy link
Member Author

kimauth commented Mar 20, 2023

It's not broken, that is the intended behaviour. (It's described in the docstring like that.)

However, after some discussion with @fredrikekre, I'll add error messages to some of the functions for the case that a MixedDofHandler has several FieldHandlers.

@termi-official
Copy link
Member

Okay. Then I have a suggestion for the dof handler merge. Do you think it makes sense to have different FieldHandlers? One for fields covering the full domain (i.e. the functionality of the current dof handler) and one for fields on subdomains (which is the selling point of the mixed dof handler)? cc @fredrikekre

@fredrikekre
Copy link
Member

That is what Kim and I discussed. We rename MixedDofHandler to DofHandler (removing the old one), and rename FieldHandler to SubDofHandler. If you are using the subdomain functionality, then you are forced to use the corresponding SubDofHandlers, but if you don't, then you can request things from the DofHandler directly. Old DofHandler would then just be new DofHandler with a single SubDofHandler, that you don't need to worry about.

@koehlerson
Copy link
Member

How should I know then externally if a field is defined on all types of cells as e.g. :v in this example: https://github.com/Ferrite-FEM/Ferrite.jl/blob/master/test/test_mixeddofhandler.jl#L437-L485 ? Do I have to loop then in any case over SubDofHandlers?

@fredrikekre fredrikekre merged commit 52ef7dc into master Mar 21, 2023
@fredrikekre fredrikekre deleted the ka/dofhandler_functions branch March 21, 2023 08:09
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.

None yet

5 participants