-
Notifications
You must be signed in to change notification settings - Fork 102
Fix PointEvalHandler on true subdomains #1182
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,7 @@ | |||||||||||||
| end | ||||||||||||||
|
|
||||||||||||||
| """ | ||||||||||||||
| PointEvalHandler(grid::Grid, points::AbstractVector{Vec{dim,T}}; kwargs...) where {dim, T} | ||||||||||||||
| PointEvalHandler(grid::Grid, points::AbstractVector{Vec{dim,T}} [, set = subdomains::Vector{OrderedSet{Int}}] ; kwargs...) where {dim, T} | ||||||||||||||
| The `PointEvalHandler` can be used for function evaluation in *arbitrary points* in the | ||||||||||||||
| domain -- not just in quadrature points or nodes. | ||||||||||||||
|
|
@@ -33,6 +33,7 @@ | |||||||||||||
| grid::G | ||||||||||||||
| cells::Vector{Union{Nothing, Int}} | ||||||||||||||
| local_coords::Vector{Union{Nothing, Vec{1, T}, Vec{2, T}, Vec{3, T}}} | ||||||||||||||
| constructed_on_subdomain::Bool | ||||||||||||||
| end | ||||||||||||||
|
|
||||||||||||||
| function Base.show(io::IO, ::MIME"text/plain", ph::PointEvalHandler) | ||||||||||||||
|
|
@@ -58,7 +59,13 @@ | |||||||||||||
| function PointEvalHandler(grid::AbstractGrid{dim}, points::AbstractVector{Vec{dim, T}}; search_nneighbors = 3, warn::Bool = true, strategy = NewtonLineSearchPointFinder()) where {dim, T} | ||||||||||||||
| node_cell_dicts = _get_node_cell_map(grid) | ||||||||||||||
| cells, local_coords = _get_cellcoords(points, grid, node_cell_dicts, search_nneighbors, warn, strategy) | ||||||||||||||
| return PointEvalHandler(grid, cells, local_coords) | ||||||||||||||
| return PointEvalHandler(grid, cells, local_coords, false) | ||||||||||||||
| end | ||||||||||||||
|
|
||||||||||||||
| function PointEvalHandler(grid::AbstractGrid{dim}, points::AbstractVector{Vec{dim, T}}, subdomains::Vector{OrderedSet{Int}}; search_nneighbors = 3, warn::Bool = true, strategy = NewtonLineSearchPointFinder()) where {dim, T} | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice if we could have a keword argument
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that a kwarg makes the user-experience better and we don't lock in on arg ordering. Even if an arg defaulting to |
||||||||||||||
| node_cell_dicts = _get_node_cell_map(grid, subdomains) | ||||||||||||||
| cells, local_coords = _get_cellcoords(points, grid, node_cell_dicts, search_nneighbors, warn, strategy) | ||||||||||||||
| return PointEvalHandler(grid, cells, local_coords, true) | ||||||||||||||
| end | ||||||||||||||
|
|
||||||||||||||
| function _get_cellcoords(points::AbstractVector{Vec{dim, T}}, grid::AbstractGrid, node_cell_dicts::Dict{C, Dict{Int, Vector{Int}}}, search_nneighbors, warn, strategy::NewtonLineSearchPointFinder) where {dim, T <: Real, C} | ||||||||||||||
|
|
@@ -202,6 +209,31 @@ | |||||||||||||
| return cell_dicts | ||||||||||||||
| end | ||||||||||||||
|
|
||||||||||||||
| function _get_node_cell_map(grid::AbstractGrid, subdomains::Vector{OrderedSet{Int}}) | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this to be its own function or could we just modify the original
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I simply prefer to use multiple dispatch over kwargs/if-else constructs using isa, because it is easier to extend later when we add new features and feels cleaner.
The field to which the point is associated might span multiple subdomains (e.g. we have the cellsets named "A", "B" and "C", then we might want to search only on "B" and "C"). Think e.g. about discretizing the torso of some kind of animal, then each organ is potentially at least one subdomain. Does that make sense?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the performance advantage sufficient to motivate the difference to other interfaces? E.g. that we don't allow users to pass a vector of facetsets to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry, but that does not make sense for me. First, I do not see what this has to do with performance advantages. Second, we already use Vector internally in Ferrite.jl/src/PointEvalHandler.jl Line 238 in 6a14d7a
All other handlers provide
I am not sure if that is true. The following code works without problem, right? ch = ...
add!(ch, Dirichlet(:v, ∂Ω1, x -> sin(x[1]))
add!(ch, Dirichlet(:v, ∂Ω2, x -> sin(x[1]))
close!(ch)Or is there something I am missing that requires us to use: ch = ...
add!(ch, Dirichlet(:v, union(∂Ω1, ∂Ω2) x -> sin(x[1]))
close!(ch)? If that is a problem, can we fix it without merging these sets?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would even argue for simplicity here to just have some second constructor here
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry, maybe I misunderstood, but I thought you are proposing supporting the equivalent of add!(ch, Dirichlet(:v, [∂Ω1, ∂Ω2], x -> sin(x[1]))
The only one I could see was avoiding creating a new set, which should be rather cheap in most cases.
I don't follow this - there is no cellset/subdomain/subdomains internally now, right?
That sounds like a good idea to me, interested to see how that would look! I just didn't think the passing of a Vector of sets was aligned with such an interface, the ability to I also like the short-hand version with the fieldname :)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not directly, which is part of the problem. See Ferrite.jl/src/PointEvalHandler.jl Lines 237 to 238 in 6a14d7a
I will try to bring this PR over the finish line early next week. |
||||||||||||||
| @inline function _check_contained_subdomain(i, subdomains) | ||||||||||||||
| for subdomain in subdomains | ||||||||||||||
| i ∈ subdomain && return true | ||||||||||||||
| end | ||||||||||||||
| return false | ||||||||||||||
| end | ||||||||||||||
|
Comment on lines
+213
to
+218
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
| cells = getcells(grid) | ||||||||||||||
| C = eltype(cells) # possibly abstract | ||||||||||||||
| cell_dicts = Dict{Type{<:C}, Dict{Int, Vector{Int}}}() | ||||||||||||||
| ctypes = Set{Type{<:C}}(typeof(c) for c in cells) | ||||||||||||||
| for ctype in ctypes | ||||||||||||||
| cell_dict = cell_dicts[ctype] = Dict{Int, Vector{Int}}() | ||||||||||||||
| for (cellidx, cell) in enumerate(cells) | ||||||||||||||
| cell isa ctype || continue | ||||||||||||||
| _check_contained_subdomain(cellidx, subdomains) || continue | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this enough, or was a performance optimization required?
Suggested change
But if following other comments, would of course also remove the |
||||||||||||||
| for node in cell.nodes | ||||||||||||||
| v = get!(Vector{Int}, cell_dict, node) | ||||||||||||||
| push!(v, cellidx) | ||||||||||||||
| end | ||||||||||||||
| end | ||||||||||||||
| end | ||||||||||||||
| return cell_dicts | ||||||||||||||
| end | ||||||||||||||
|
|
||||||||||||||
| """ | ||||||||||||||
| evaluate_at_points(ph::PointEvalHandler, dh::AbstractDofHandler, dof_values::Vector{T}, [fieldname::Symbol]) where T | ||||||||||||||
| evaluate_at_points(ph::PointEvalHandler, proj::L2Projector, dof_values::Vector{T}) where T | ||||||||||||||
|
|
@@ -226,6 +258,9 @@ | |||||||||||||
| ph::PointEvalHandler{<:Any, T1}, dh::AbstractDofHandler, dof_vals::AbstractVector{T2}, | ||||||||||||||
| fname::Symbol = find_single_field(dh) | ||||||||||||||
| ) where {T1, T2} | ||||||||||||||
| if !is_defined_on_full_domain(dh) && !ph.constructed_on_subdomain | ||||||||||||||
| @warn "DofHandler is defined on a true subdomain only, but the PointEvalHandler serached cells on the full grid. The evaluation assignment might be broken!" maxlog = 1 | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
| end | ||||||||||||||
| npoints = length(ph.cells) | ||||||||||||||
| # Figure out the value type by creating a dummy PointValues | ||||||||||||||
| ip = getfieldinterpolation(dh, find_field(dh, fname)) | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| using Ferrite, Test | ||
| using Ferrite, Test, Logging, OrderedCollections | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this even here, I don't think we do this consistently?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure why these are missing from the test. Running only parts of the tests is right now quite painful, as we include some of the deps in the runner and sometime some of the deps in the file containing the testsets. Would you agree that it would be better if the different files containing the test sections would be self-contained, so we can run them directly for testing instead of modifying the test codes every time we only want to test part of the API?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I totally agree that this can be very useful, I was just not aware that we have this as a convention to do! Would of course then be nicer if it would error if one doesn't do that, otherwise it is quite hard to make sure it is done consistently. This also includes e.g. including
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea we should probably follow something like this here https://github.com/SciML/OrdinaryDiffEq.jl/blob/1804a1bbc436c88580310316e69ff5a091f65eb1/test/runtests.jl , especially if we start expanding convergence rate tests and GPU test coverage. |
||
|
|
||
| function test_pe_scalar_field() | ||
| # isoparametric approximation | ||
|
|
@@ -345,16 +345,15 @@ function test_pe_mixed_grid() | |
| # construct projector | ||
| projector = L2Projector(ip_quad, mesh; set = getcellset(mesh, "quads")) | ||
|
|
||
| points = [Vec((x, 2x)) for x in range(0.0; stop = 1.0, length = 10)] | ||
| points = [Vec((x, 2x)) for x in range(0.0; stop = 0.5, length = 5)] | ||
|
|
||
| # first alternative: L2Projection to dofs | ||
| projector_values = project(projector, qp_vals_quads, qr) | ||
| @test_logs min_level = Logging.Warn PointEvalHandler(mesh, points) | ||
| ph = PointEvalHandler(mesh, points) | ||
| ph = PointEvalHandler(mesh, points, [getcellset(mesh, "quads")]) | ||
| @test all(x -> x !== nothing, ph.cells) | ||
| vals = evaluate_at_points(ph, projector, projector_values) | ||
| @test vals[1:5] ≈ f.(points[1:5]) | ||
| @test all(isnan, vals[6:end]) | ||
|
|
||
| # second alternative: assume a vector field :v | ||
| dh = DofHandler(mesh) | ||
|
|
@@ -427,6 +426,25 @@ function test_pe_first_point_missing() | |
| return | ||
| end | ||
|
|
||
| function test_pe_true_subdomain() | ||
| # Generate a subdofhandler on two elements | ||
| grid = generate_grid(Triangle, (4, 4), Vec((-2.0, -2.0)), Vec((2.0, 2.0))) | ||
| addcellset!(grid, "hole", x -> norm(x) ≤ 1.0) | ||
| subdomain = getcellset(grid, "hole") | ||
| dh = DofHandler(grid) | ||
| sdh = SubDofHandler(dh, subdomain) | ||
| add!(sdh, :v, Lagrange{RefTriangle, 1}()) | ||
| close!(dh) | ||
|
|
||
| # Evaluate at all nodes of all triangles in "hole" | ||
| nodes = Vec{2, Float64}.([[0.0, -1.0], [0.0, 0.0], [-1.0, 0.0], [1.0, 0.0], [0.0, 1.0]]) | ||
| ph = PointEvalHandler(grid, nodes, [subdomain]; warn = true) | ||
| result = Ferrite.evaluate_at_points(ph, dh, zeros(ndofs(dh))) | ||
|
|
||
| @test !any(isnan.(result)) | ||
| return | ||
| end | ||
|
|
||
| @testset "PointEvalHandler" begin | ||
| @testset "scalar field" begin | ||
| test_pe_scalar_field() | ||
|
|
@@ -458,6 +476,10 @@ end | |
| @testset "failure cases" begin | ||
| test_pe_first_point_missing() | ||
| end | ||
|
|
||
| @testset "True Subdomain (issue #1181)" begin | ||
| test_pe_true_subdomain() | ||
| end | ||
| end | ||
|
|
||
| @testset "PointValues" begin | ||
|
|
||
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.
Wouldn't you have the same problem with a field that's defined on a subdomain, even if all cells are part of some
SubDofHandler? In that case this check isn't sufficient, is it?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.
Good catch, you are right. Let me fix that.