Rework CellIterator by extracting out the caching part #546
Conversation
fredrikekre
added
enhancement
refactoring
|
Nice! |
| throw(ArgumentError(msg)) | ||
| end | ||
| # reinit! FEValues with CellCache | ||
| reinit!(cv::CellValues, cc::CellCache) = reinit!(cv, cc.coords) |
There was a problem hiding this comment.
Personally i never liked this method...I think it hides to much information to new users.
Using reinit!(cv, getcoordinates(cc)) is clearer i think.
There was a problem hiding this comment.
Sure, but could be in a cleanup PR, this is just replicating the functionality we already have :)
| cellnodes!(cc.nodes, cc.grid, i) | ||
| end | ||
| if cc.flags.coords | ||
| cellcoords!(cc.coords, cc.grid, i) |
There was a problem hiding this comment.
Here you can change to getcoordinates, since dh isa DofHandler and has concrete cell type? And we will probably soon deprecate cellcoords!
There was a problem hiding this comment.
There doesn't seem to be a method of getcoordinates that take a pre-allocated vector as input?
There was a problem hiding this comment.
https://github.com/Ferrite-FEM/Ferrite.jl/blob/master/src/Grid/grid.jl#L668-L685 if I understand lijas correctly?
There was a problem hiding this comment.
Edit: Beaten by @termi-official :)
getcoordinates!?
| @assert cc.dh isa MixedDofHandler | ||
| cc.cellid[] = i | ||
| if cc.flags.nodes | ||
| resize!(cc.nodes, nnodes_per_cell(cc.dh, i)) |
There was a problem hiding this comment.
This is new behavior, since previously we would have gotten an error if we loop over a cellset with different cell types. Maybe this is better 🤔 @kimauth ?
There was a problem hiding this comment.
I think the error would just happen later in that case, e.g. when trying to reinit! FE values with the wrong size. The CellIterator(...) approach still checks that all elements have the same type too.
This patch reworks the CellIterator by extracting out the caching parts to its own CellCache struct that can be used independently. Elements of a CellIterator are now CellCache instead of the CellIterator itself.
Codecov ReportBase: 93.02% // Head: 92.26% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #546 +/- ##
==========================================
- Coverage 93.02% 92.26% -0.77%
==========================================
Files 23 22 -1
Lines 3973 3530 -443
==========================================
- Hits 3696 3257 -439
+ Misses 277 273 -4
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 at Codecov. |
This patch reworks the CellIterator by extracting out the caching parts to its own CellCache struct that can be used independently. Elements of a CellIterator are now CellCache instead of the CellIterator itself.