-
Notifications
You must be signed in to change notification settings - Fork 89
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
Rework CellIterator by extracting out the caching part #546
Conversation
Nice! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.