-
Notifications
You must be signed in to change notification settings - Fork 86
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
Improve get_coordinates!
#536
Conversation
Codecov ReportBase: 92.56% // Head: 92.56% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #536 +/- ##
=======================================
Coverage 92.56% 92.56%
=======================================
Files 22 22
Lines 3900 3901 +1
=======================================
+ Hits 3610 3611 +1
Misses 290 290
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. |
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.
Cool addition! Thanks for tackling this. I left some minor comments.
I think if you have an explicit |
Hearing that makes me less suspicious about my benchmark results. 😅 It also makes a strong point for actually removing the coordinate representation from the |
I think we can remove the coordinate representation in the |
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.
Very nice changes! Looking forward to have the DofHandlers merged :)
PR
Regular Grid:
getcoordinates(xe, grid, cellid): 9.023 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, grid, cellid): 9.033 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, dh, cellid): 9.034 ns (0 allocations: 0 bytes)
Mixed Grid, AbstractCell
getcoordinates(xe, grid, cellid): 27.389 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, grid, cellid): 28.400 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, dh, cellid): 9.371 ns (0 allocations: 0 bytes)
Mixed Grid, Union{Quadrilateral, Triangle, Line}
getcoordinates(xe, grid, cellid): 9.705 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, grid, cellid): 10.712 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, dh, cellid): 9.370 ns (0 allocations: 0 bytes)
Mixed Grid, Union{Quadrilateral, Triangle, Line, Quadratic}
getcoordinates(xe, grid, cellid): 10.037 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, grid, cellid): 11.363 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, dh, cellid): 9.369 ns (0 allocations: 0 bytes)
Mixed Grid, Union{Quadrilateral, Triangle, Line, Quadratic, Line2D}
getcoordinates(xe, grid, cellid): 11.056 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, grid, cellid): 12.695 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, dh, cellid): 9.120 ns (0 allocations: 0 bytes)
Master
Regular Grid:
getcoordinates(xe, grid, cellid): 9.656 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, grid, cellid): 10.425 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, dh, cellid): 10.882 ns (0 allocations: 0 bytes)
Mixed Grid, AbstractCell
getcoordinates(xe, grid, cellid): 2.504 μs (20 allocations: 704 bytes)
Ferrite.cellcoords!(xe, grid, cellid): 2.755 μs (14 allocations: 464 bytes)
Ferrite.cellcoords!(xe, dh, cellid): 10.814 ns (0 allocations: 0 bytes)
Mixed Grid, Union{Quadrilateral, Triangle, Line}
getcoordinates(xe, grid, cellid): 12.197 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, grid, cellid): 18.511 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, dh, cellid): 10.499 ns (0 allocations: 0 bytes)
Mixed Grid, Union{Quadrilateral, Triangle, Line, Quadratic}
getcoordinates(xe, grid, cellid): 12.138 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, grid, cellid): 19.279 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, dh, cellid): 10.812 ns (0 allocations: 0 bytes)
Mixed Grid, Union{Quadrilateral, Triangle, Line, Quadratic, Line2D}
getcoordinates(xe, grid, cellid): 15.337 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, grid, cellid): 20.822 ns (0 allocations: 0 bytes)
Ferrite.cellcoords!(xe, dh, cellid): 10.503 ns (0 allocations: 0 bytes)
@inbounds for i in 1:length(x) | ||
x[i] = grid.nodes[grid.cells[cell].nodes[i]].x | ||
x[i] = getcoordinates(getnodes(grid, cell.nodes[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 not for this PR, but in general a Cell
Interface function to return the nodes would be nice, like getnodes(cell::AbstractCell,i::Integer)
I think there is none currently
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 are indeed a few things which can be improved regarding the element and mesh interface, but I would leave them to separate PRs.
Do you think it makes sense to put some information of this PR into the docs into a performance section? |
Mentioning that the cell vector in a |
Perhaps also worth mentioning that a half-typed |
I'll leave the deprecation for a future PR, as there are more things to deprecate on the way to merging the dofhandlers. That leaves this PR as purely changing internals. |
Small improvements on the definition of
getcoordinates!
. In particular:AbstractGrid
interface . See theSmallGrid
example from the docs, that doesn't need to definecellcoords!
anymoregetcoordinates!
working on grids andcellcoords!
working on dofhandlers (but also on grids...). ForDofHandler
both fall back to one definition with this PR. We should deprecate one of them in the future.Coordinate query benchmarks
This PR is part of making an effort to join the two different dofhandlers. Therefore, here are some interesting performance investigations on querying coordinates:
Regular grid
The difference on a standard non-mixed grid is minor:
getcoordinates!(xe, grid, cellid)
Ferrite.cellcoords!(xe, grid, cellid)
Ferrite.cellcoords!(xe, dh, cellid)
Mixed grid
The PR performs much better than master on querying coordinates directly from the grid for a cell vector of type
AbstractCell
. However, that is still about twice as slow compared to querying coordinates from theMixedDofHandler
.AbstractCell
getcoordinates!(xe, grid, cellid)
Ferrite.cellcoords!(xe, grid, cellid)
Ferrite.cellcoords!(xe, dh, cellid)
Notice:
For mixed grids (and thus
MixedDofHandler
)cellcoords!(xe, dh, cellid)
is unchanged between the PR and master.The more interesting result is that when using a union type for the cell vector instead of
AbstractCell
, the coordinate query from the grid is almost as fast as for a non-mixed grid and faster than querying the coordinates from theMixedDofHandler
. (In fact, this has been the case even on master.)I got similar results for grids with 4 and 5 different cell types, compare the benchmark code below. This surprised me, because I thought this worked well due to union splitting, which seems to cover up to 4 types. Perhaps someone can explain that. 🙂
Union{Quadrilateral, Triangle, Line}
getcoordinates!(xe, grid, cellid)
Ferrite.cellcoords!(xe, grid, cellid)
Ferrite.cellcoords!(xe, dh, cellid)
Take away message:
As long as the cell vector is well typed, there seems to be no need for a specialized coordinate query for mixed grids (at least up to 5 different cell types). That would mean we could remove the grid representation from the
MixedDofHandler
and have it fall back to querying the coordinates directly from its grid as well. That would be a major step towards joining the dofhandlers.Benchmark code