-
Notifications
You must be signed in to change notification settings - Fork 92
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
ExclusiveTopology with ArrayOfVectorViews backend #974
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #974 +/- ##
==========================================
+ Coverage 93.20% 93.58% +0.37%
==========================================
Files 38 39 +1
Lines 5816 5888 +72
==========================================
+ Hits 5421 5510 +89
+ Misses 395 378 -17 ☔ View full report in Codecov by Sentry. |
c276cb2 brings construction in benchmark further down from |
When rechecking the timings, I saw some worse performance now. Scriptfunction analyze_top_sizes(grid; show_avg=false)
top = ExclusiveTopology(grid);
for key in fieldnames(typeof(top))
nbh = getfield(top, key)
nbh isa Ferrite.ArrayOfVectorViews || continue
max_size = maximum(length, nbh; init=0)
avg_size = sum(length, nbh; init=0)/length(nbh)
if show_avg
println(key)
println(" max: ", max_size, " entities")
println(" avg: ", avg_size, " entities")
else
println(key, ": ", max_size)
end
end
end
function print_sizes(n=10, celltypes=[Line, Triangle, Quadrilateral, Tetrahedron, Hexahedron])
for CT in celltypes
grid = generate_grid(CT, ntuple(_->n, Ferrite.getrefdim(CT)))
println(CT)
analyze_top_sizes(grid)
println()
end
end Max number of entities for different generated grids
|
I will fix #988 after merging this PR to avoid annoying conflicts. |
Resolves #617 ? |
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.
Great improvement on the performance and I think the data structure will also help in the long run. I left a few comments.
Co-authored-by: Dennis Ogiermann <termi-official@users.noreply.github.com>
Thanks! |
Co-authored-by: Maximilian Köhler <maximilian.koehler@ruhr-uni-bochum.de>
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.
LGTM. Can you dump the benchmark code in the benchmark folder (here or separate PR)?
Thanks! I haven't looked so much at the benchmark setup, will just open an issue for now to not forget to do it later unless someone solves it before I have time. |
See discussion in #965, this PR introduces a cleaned-up version.
Most important changes in this PR compared to master are:
Array{EntityNeighborhood{T}, N}
->ArrayOfVectorViews{T, N} <: AbstractArray{"View{Vector{T}}", N}
getcells
dispatches removed (since these would now act onVector{<:BoundaryIndex}
). In any case,getcells
normally returns::AbstractCell
s, but in topology they return::Int
s on mater, if any,cellid
should be overloaded).EntityNeighborhood
is goneCompared to #965, the support for hashed indexing is removed and only
ExclusiveTopology
is implemented. Before finalizing, I would like some feedback from the topology side, and then I'll addArrayOfVectorViews
ArrayOfVectorViews
ExclusiveTopology
Let me know if more details are required for the initial review.
Note that in relation to #942, this PR explicitly errors if trying to construct
ExclusiveTopology
with grids containing embedded cells, thus avoiding silent bugs.Benchmarks
(PR refers to b7e2add)