Refactor to use grid and dofhandler interface internally #655

termi-official · 2023-03-29T14:53:41Z

First cherry-pick from #486 . Basically uses the interface for the types instead of accessing members of them directly. This is used to allow local views of distributed grids and distributed dof handlers to be used (non-overlapping case!).

…easonable - rolling back one change.

src/Grid/grid.jl

codecov-commenter · 2023-03-29T20:39:19Z

Codecov Report

Patch coverage: 91.54% and project coverage change: -0.02 ⚠️

Comparison is base (185f8d6) 92.27% compared to head (b65284f) 92.25%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #655      +/-   ##
==========================================
- Coverage   92.27%   92.25%   -0.02%     
==========================================
  Files          30       30              
  Lines        4594     4598       +4     
==========================================
+ Hits         4239     4242       +3     
- Misses        355      356       +1

Impacted Files	Coverage Δ
src/Grid/grid.jl	`92.56% <33.33%> (-0.18%)`	⬇️
src/Dofs/DofHandler.jl	`92.73% <80.00%> (+0.04%)`	⬆️
src/iterators.jl	`88.33% <87.50%> (ø)`
src/Dofs/ConstraintHandler.jl	`95.97% <100.00%> (+<0.01%)`	⬆️
src/Dofs/apply_analytical.jl	`100.00% <100.00%> (ø)`
src/Export/VTK.jl	`94.82% <100.00%> (ø)`
src/L2_projection.jl	`100.00% <100.00%> (ø)`

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

KnutAM

LGTM
(But I think it would make sense to use getcelltype in a few places)

src/Dofs/ConstraintHandler.jl

src/Dofs/apply_analytical.jl

Co-authored-by: Knut Andreas Meyer <knutam@gmail.com>

termi-official · 2023-03-30T10:44:41Z

Seems like I forgot some lines. Your suggestion pointed me to them. Thanks!

KnutAM · 2023-03-30T11:14:09Z

src/iterators.jl

@@ -113,7 +113,7 @@ cellid(cc::CellCache) = cc.cellid[]
 celldofs!(v::Vector, cc::CellCache) = copyto!(v, cc.dofs) # celldofs!(v, cc.dh, cc.cellid[])

 # TODO: These should really be replaced with something better...
-nfaces(cc::CellCache) = nfaces(eltype(cc.grid.cells))
+nfaces(cc::CellCache) = nfaces(getcelltype(cc.grid))


This fails for non-concrete grids, but unrelated to this pr...
But should be relevant for merging of dofhandlers, cc: @kimauth

But should be easy to fix.

Suggested change

nfaces(cc::CellCache) = nfaces(getcelltype(cc.grid))

nfaces(cc::CellCache) = nfaces(getcelltype(cc.grid, cc.cellid.x))

Yes, I'm aware of this. Not an issue for merging, as this will still work as long as the grid is concrete (so it works for people who are using DofHandler now.
Should benchmark that solution, it has the usual problem of extracting an unconcrete cell, which might or might not be an issue.
I think nfaces(fv::FaceValues) should be used instead in most cases.

Exactly, so I suggest not changing that in this pr!

KnutAM

I cannot find any remaining dh.grid in src/

fredrikekre · 2023-03-30T13:06:58Z

I have to say that I find

xj = grid.nodes[nodes[j]].x

much more readable than

xj = getcoordinates(getnodes(grid, nodes[j]))

. Methods that are typed with ::Grid or ::DofHandler are not generic anyway, and many methods that are typed ::AbstractDofHandler should really be ::DofHandler since they use the internal representation of that specific implementation.

koehlerson · 2023-03-30T13:33:37Z

. Methods that are typed with ::Grid or ::DofHandler are not generic anyway

I disagree with this, you can assemble and solve the heat equation by subtyping AbstractGrid https://github.com/Ferrite-FEM/Ferrite.jl/blob/master/test/test_abstractgrid.jl

termi-official · 2023-03-30T13:37:25Z

Actually I do not think that this is a question of readability here (because readability of a user often comes from what he is used to read and write).

However, I agree that some methods are specific on our Grid. Most of them could be easily generalized to other grid types by simply enforcing the interface (e.g. a grid which does not use Node but something else, or a grid holding a grid as in the distributed assembler), which is the reason why I tried to enforce the interface internally to the best of my skills.

fredrikekre · 2023-03-30T14:08:20Z

I disagree with this, you can assemble and solve the heat equation by subtyping AbstractGrid

Yes, but you are not hitting any of the code paths that (currently) uses ::Grid then. But for example, if you have a method with ::ConstraintHandler, that will always have a dh field pointing to the dofhandler, just like ::DofHandler will always have a field grid pointing to the grid (and I ought to be allowed to use it :D )

and many methods that are typed ::AbstractDofHandler should really be ::DofHandler since they use the internal representation of that specific implementation.

With this I mean that for example

Ferrite.jl/src/Dofs/MixedDofHandler.jl

Line 82 in b019eea

ndofs(dh::AbstractDofHandler) = dh.ndofs[]

should be ::MixedDofHandler and not ::AbstractDofHandler -- the interface should be to define ndofs(::MyDofHandler) not to have this specific field:

Ferrite.jl/src/Dofs/MixedDofHandler.jl

Line 54 in b019eea

ndofs::ScalarWrapper{Int}

koehlerson · 2023-03-30T14:19:06Z

I disagree with this, you can assemble and solve the heat equation by subtyping AbstractGrid

Yes, but you are not hitting any of the code paths that (currently) uses ::Grid then. But for example, if you have a method with ::ConstraintHandler, that will always have a dh field pointing to the dofhandler, just like ::DofHandler will always have a field grid pointing to the grid (and I ought to be allowed to use it :D )

I think it's the other way around. We are hitting code paths that used to use ::Grid which incrementally changed to ::AbstractGrid, e.g. #379 Here the main changes are to periodic BCs in the ConstraintHandler that has been added without <:AbstractGrid support, which is not problematic, but can be changed without any problems, because the periodic bc are then simply richer in features, aren't they?

For AbstractDofHandler I agree that the discussion is different since there isn't a collection of interface functions which make a AbstractDofHandler work and is tested (afaik). Nonetheless it should be discussed if this should be the goal if we think of distributed or adaptive things

termi-official · 2023-03-30T14:32:52Z

But you are hitting code paths that uses <:AbstractGrid. Here I am not seeing why we should artificially restrict these code paths by enforcing specific variable names (and types) in functions like reshape_to_nodes or even add!, when they should just work out fine if some OtherGrid <: AbstractGrid fullfills the interface. I mean, yes, we can just copy paste the codes and change for example grid.nodes to grid.mynodes (and friends) for OtherGrid, but I am completely lost to see why this is beneficial over just dispatching getnodes (and friends). May you elaborate here?

KnutAM · 2023-03-30T15:27:47Z

I have to say that I find
xj = grid.nodes[nodes[j]].x
much more readable than
xj = getcoordinates(getnodes(grid, nodes[j]))

I agree which is why I suggested depreciating getcoordinates in favor of get_cell_coordinates and get_node_coordinate in #394 (comment), making it

xj = get_node_coordinate(grid, nodes[j])

But that should be left for a separate PR.

👍 for this:

With this I mean that for example

Ferrite.jl/src/Dofs/MixedDofHandler.jl

Line 82 in b019eea

ndofs(dh::AbstractDofHandler) = dh.ndofs[]

should be ::MixedDofHandler and not ::AbstractDofHandler

For larger functions, I think it makes sense to use the accessor functions even if direct field access can be used, making it easier to generalize if required later. Of course, ideally this should happen at once with a TestGrid<:AbstractGrid for testing purposes.
The alternative is what I did in #579 by only changing (I think) in places where AbstractGrid is allowed (and that only considers the grid API). But I think this PR includes almost everything that #579 does (perhaps except the simplifications in the access functions)

fredrikekre

LGTM but see the comment about exporting the new names (and various other things).

docs/src/devdocs/dofhandler.md

fredrikekre · 2023-06-10T06:20:37Z

src/Dofs/ConstraintHandler.jl

-    if interpolation !== default_interpolation(typeof(ch.dh.grid.cells[first(cellset)]))
+function _add!(ch::ConstraintHandler, dbc::Dirichlet, bcnodes::Set{Int}, interpolation::Interpolation, field_dim::Int, offset::Int, bcvalue::BCValues, cellset::Set{Int}=Set{Int}(1:getncells(getgrid(ch.dh))))
+    grid = getgrid(ch.dh)
+    if interpolation !== default_interpolation(getcelltype(grid, first(cellset)))


getcelltype(grid, i) should probably be typeof(getcell(grid, i)). Although, getcelltype` can be optimized for homogeneous grids, but not sure that matters.

I added it because I thought it is a nice convenience which is clearer in the intent.

src/Dofs/DofHandler.jl

src/exports.jl

Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

KnutAM · 2023-06-10T18:53:05Z

I'll hold #664 until this one is merged (I think that should be the least amount of work regarding conflict resolution)

termi-official added 4 commits March 29, 2023 15:07

First sweep, mostly for abstracting grid access via dof handlers.

30fa9d5

Fix oopsies.

ce09d34

More cleanup.

2835012

I think keeping the assumption that grids come with nodal spaces is r…

fc3b69e

…easonable - rolling back one change.

KnutAM reviewed Mar 29, 2023

View reviewed changes

src/Grid/grid.jl Show resolved Hide resolved

Get coordinate type directly from grid.

f18d86b

KnutAM approved these changes Mar 30, 2023

View reviewed changes

src/Dofs/ConstraintHandler.jl Outdated Show resolved Hide resolved

src/Dofs/apply_analytical.jl Outdated Show resolved Hide resolved

src/Dofs/apply_analytical.jl Outdated Show resolved Hide resolved

termi-official and others added 2 commits March 30, 2023 11:16

Apply suggestions from code review

aa4cc42

Co-authored-by: Knut Andreas Meyer <knutam@gmail.com>

Fix oopsie and propagate interface in remaining code (hopefully).

4cdc697

termi-official requested a review from KnutAM March 30, 2023 10:44

KnutAM reviewed Mar 30, 2023

View reviewed changes

KnutAM approved these changes Mar 30, 2023

View reviewed changes

This was referenced Mar 31, 2023

AbstractGrid #662

Closed

get_*_coordinates[!] - consistent naming #664

Merged

termi-official added 3 commits June 7, 2023 15:53

Merge branch 'master' into do/enforce-internal-grid-interface

f182e4c

Export getgrid.

ad8941a

More exports.

cc509df

termi-official mentioned this pull request Jun 7, 2023

Non-overlapping dof handler Ferrite-FEM/FerriteDistributed.jl#7

Merged

3 tasks

termi-official requested a review from fredrikekre June 7, 2023 23:00

Derp.

5914eab

fredrikekre approved these changes Jun 10, 2023

View reviewed changes

termi-official and others added 5 commits June 10, 2023 11:18

getgrid to get_grid

303513e

Revert export of getdim.

5d663a9

Apply suggestions from code review

75a3f97

Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

Docs

0f4908d

Oopsie.

5951b79

Remote get_grid export.

b65284f

fredrikekre merged commit f22d24b into master Jun 13, 2023

fredrikekre deleted the do/enforce-internal-grid-interface branch June 13, 2023 12:17

KnutAM mentioned this pull request Jul 20, 2023

getcoordinates #757

Merged

KnutAM mentioned this pull request May 16, 2024

Generalize CSC assembler #916

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to use grid and dofhandler interface internally #655

Refactor to use grid and dofhandler interface internally #655

termi-official commented Mar 29, 2023

codecov-commenter commented Mar 29, 2023 •

edited

Loading

KnutAM left a comment

termi-official commented Mar 30, 2023

KnutAM Mar 30, 2023

termi-official Mar 30, 2023 •

edited

Loading

kimauth Mar 30, 2023

KnutAM Mar 30, 2023

KnutAM left a comment

fredrikekre commented Mar 30, 2023

koehlerson commented Mar 30, 2023

termi-official commented Mar 30, 2023

fredrikekre commented Mar 30, 2023

koehlerson commented Mar 30, 2023 •

edited

Loading

termi-official commented Mar 30, 2023 •

edited

Loading

KnutAM commented Mar 30, 2023 •

edited

Loading

fredrikekre left a comment

fredrikekre Jun 10, 2023

termi-official Jun 10, 2023

KnutAM commented Jun 10, 2023

	nfaces(cc::CellCache) = nfaces(getcelltype(cc.grid))
	nfaces(cc::CellCache) = nfaces(getcelltype(cc.grid, cc.cellid.x))

Refactor to use grid and dofhandler interface internally #655

Refactor to use grid and dofhandler interface internally #655

Conversation

termi-official commented Mar 29, 2023

codecov-commenter commented Mar 29, 2023 • edited Loading

Codecov Report

KnutAM left a comment

Choose a reason for hiding this comment

termi-official commented Mar 30, 2023

KnutAM Mar 30, 2023

Choose a reason for hiding this comment

termi-official Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

kimauth Mar 30, 2023

Choose a reason for hiding this comment

KnutAM Mar 30, 2023

Choose a reason for hiding this comment

KnutAM left a comment

Choose a reason for hiding this comment

fredrikekre commented Mar 30, 2023

koehlerson commented Mar 30, 2023

termi-official commented Mar 30, 2023

fredrikekre commented Mar 30, 2023

koehlerson commented Mar 30, 2023 • edited Loading

termi-official commented Mar 30, 2023 • edited Loading

KnutAM commented Mar 30, 2023 • edited Loading

fredrikekre left a comment

Choose a reason for hiding this comment

fredrikekre Jun 10, 2023

Choose a reason for hiding this comment

termi-official Jun 10, 2023

Choose a reason for hiding this comment

KnutAM commented Jun 10, 2023

codecov-commenter commented Mar 29, 2023 •

edited

Loading

termi-official Mar 30, 2023 •

edited

Loading

koehlerson commented Mar 30, 2023 •

edited

Loading

termi-official commented Mar 30, 2023 •

edited

Loading

KnutAM commented Mar 30, 2023 •

edited

Loading