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

let DofHandler take any AbstractGrid subtype #307

Closed
wants to merge 15 commits into from

Conversation

koehlerson
Copy link
Member

@koehlerson koehlerson commented Dec 1, 2020

This is essentially the same as https://github.com/KristofferC/JuAFEM.jl/pull/302 , but instead of MixedDofHandler done for DofHandler

The DofHandler should work for any properly subtyped AbstractGrid. Obviously, one could subtype AbstractDofHandler but this is in a lot of cases unnecessary because the DofHandler is pretty robust. In our case we want just to swap the Grid datastructure.

The new parametrisation is open for discussion, but we want to reuse the DofHandler for adaptivity and thus, need the proposed flexibility w.r.t. the grid type

As pointed out in https://github.com/KristofferC/JuAFEM.jl/pull/302 I'd like the most something like DofHandler{Grid{dim, C, T}} for the existing stuff, which would lead to DofHandler{G<:AbstractGrid}, but I'm unsure and need feedback on this. The first commit uses this option: DofHandler{dim,C,T,G <: AbstractGrid}

Edit: Second commit includes DofHandler{G <: AbstractGrid, T} option

@codecov-io
Copy link

codecov-io commented Dec 1, 2020

Codecov Report

Merging #307 (5db9b04) into master (96ae0b6) will decrease coverage by 1.41%.
The diff coverage is 41.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
- Coverage   85.80%   84.38%   -1.42%     
==========================================
  Files          21       21              
  Lines        2000     2036      +36     
==========================================
+ Hits         1716     1718       +2     
- Misses        284      318      +34     
Impacted Files Coverage Δ
src/Dofs/ConstraintHandler.jl 88.77% <ø> (ø)
src/Export/VTK.jl 66.66% <9.09%> (-10.76%) ⬇️
src/Dofs/DofHandler.jl 77.52% <41.37%> (-5.65%) ⬇️
src/iterators.jl 68.65% <69.23%> (-10.30%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96ae0b6...5db9b04. Read the comment docs.

@kimauth
Copy link
Member

kimauth commented Dec 2, 2020

Maybe this is a little off-topic here - but why do you want to use the DofHandler in adaptivity and not the MixedDofHandler? Would it still be possible to use your adaptivity implementation with grids that have different cell types in them? (Or does this make no sense? )

@koehlerson
Copy link
Member Author

Ultimately, I want to support both cases that's why I opened the discussion for DofHandler as well

@koehlerson
Copy link
Member Author

This branch can now reproduce the heat equation with any subtyped AbstractGrid that implements the interface given in grid.jl.

Maybe some interesting insights from this:

  • some additional dispatches were needed, e.g. cellcoords, cellnodes
  • included a fallback vtk_grid method for AbstractGrid which only exports homogeneous grids in the sense of only one celltype inside the grid
  • had to change the CellIterator constructors

Screenshot_20201211_200318

The screenshot shows the solution of the heat-equation example, but instead of Grid we used the subtyped version, which should become adaptive soon :)

@lijas
Copy link
Collaborator

lijas commented Jan 21, 2021

Since DofHandler is Kristoffers and Fredriks baby, they get to decide if this should get merged 😄 I mainly use MixedDofHandler anyways...

@koehlerson
Copy link
Member Author

Actually, it's not so important anymore. We thought initially that we can reuse DofHandler and MixedDofHandler. But when using an adapted nonconforming grid, we need to shift some dofs, since not all distributed dofs are true dofs. Some of them need to be constrained. This is why the ConstraintHandler needs some changes, when using the new adaptive grid type. So, currently the plan is to write at some point a NCDofHandler that is essentially built on top of MixedDofHandler with some additional dispatches for the ConstraintHandler

However, the change in CellIterator is quite important. Your PR let the MixedDofHandler accept now a subtype of AbstractGrid but the CellIterator is still restricted to Grid. So, you shouldn't be able to iterate over the grid if you use a custom subtyped AbstractGrid. The same holds true for the fallback methods of cellcoords!,cellnodes!

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.

4 participants