Skip to content

Adding a couple of broken tests to DivConformingFESpacesTests #1292

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.

Sign up for GitHub

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

Closed
amartinhuertas wants to merge 4 commits into from
+40 −0
Closed

Adding a couple of broken tests to DivConformingFESpacesTests #1292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a1d952
Adding a couple of broken tests to DivConformingFESpacesTests.
amartinhuertas Apr 29, 2026
a363ba1
Added PR # to NEWS.md
amartinhuertas Apr 29, 2026
6b06b5c
Do not use rand in the added broken tests, as suggested by CoPilot
amartinhuertas Apr 29, 2026
ba3cbd4
Update test/FESpacesTests/DivConformingFESpacesTests.jl
amartinhuertas Apr 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added
- A couple of broken tests in DivConformingFESpacesTests.jl related to NormalSignMap facet cell owners default criterion. Since PR[#1292](https://github.com/gridap/Gridap.jl/pull/1292)
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NEWS entry reads like this change happened “Since PR #1292”, but the tests are being added in this PR; PR #1292 seems to be the regression point. Consider rewording to something like “Added broken tests tracking a regression introduced in PR #1292” (and optionally align with the usual spacing style PR [#1292]).

Suggested change
- A couple of broken tests in DivConformingFESpacesTests.jl related to NormalSignMap facet cell owners default criterion. Since PR[#1292](https://github.com/gridap/Gridap.jl/pull/1292)
- Added a couple of broken tests in `DivConformingFESpacesTests.jl` tracking a regression introduced in PR [#1292](https://github.com/gridap/Gridap.jl/pull/1292).

Copilot uses AI. Check for mistakes.

Comment thread
amartinhuertas marked this conversation as resolved.
## [0.20.5] - 2026-04-28

### Fixed
Expand Down
37 changes: 37 additions & 0 deletions test/FESpacesTests/DivConformingFESpacesTests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,4 +294,41 @@ test_div_v_q_equiv(U,V,P,Q,Ω)

end

# This test checks that the facet owner cell criterion
# underlying the normal sign map is consistent no matter
# how cells sharing a facet are listed in the model.
Comment on lines +297 to +299
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test comment says the facet-owner criterion should be consistent regardless of how adjacent cells are listed, but NormalSignMap is currently documented/implemented as choosing the owner as the first adjacent cell in facet_to_cell ordering. Since these assertions are intentionally @test_broken, it would help to link to a tracking issue/PR (or explicitly state this is a desired behavior change) so it’s clear this is a planned semantic change rather than an out-of-spec topology mutation.

Suggested change
# This test checks that the facet owner cell criterion
# underlying the normal sign map is consistent no matter
# how cells sharing a facet are listed in the model.
# Desired behavior change for `NormalSignMap`:
# the facet owner criterion underlying the normal sign map should
# be invariant to the order in which cells sharing a facet are listed
# in the model. The current documented/implemented behavior selects the
# owner as the first adjacent cell in `facet_to_cell` ordering, so the
# assertions below are intentionally marked `@test_broken` to document
# the intended semantics rather than the current behavior.

Copilot uses AI. Check for mistakes.
@testset "NormalSignMap" begin

# Create domain
domain = (0,1,0,1)
cells = (2,1)
model = CartesianDiscreteModel(domain,cells)
order = 0
reffe = ReferenceFE(raviart_thomas,Float64,order)
V1 = TestFESpace(model,reffe)
uh1 = FEFunction(V1,ones(num_free_dofs(V1)))

topo = get_grid_topology(model)
facet_cells = get_faces(topo,1,2)
interior_facets = 0
for facet in 1:(length(facet_cells.ptrs)-1)
first_cell = facet_cells.ptrs[facet]
last_cell = facet_cells.ptrs[facet+1] - 1
if last_cell - first_cell + 1 == 2
facet_cells.data[first_cell], facet_cells.data[last_cell] =
facet_cells.data[last_cell], facet_cells.data[first_cell]
interior_facets += 1
end
end
@test interior_facets == 1
V2 = TestFESpace(model,reffe)
Comment on lines +299 to +324
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mutating facet_cells.data here modifies the UnstructuredGridTopology cached face-to-cell table inside model (since get_faces returns the cached table). This makes the topology state of model inconsistent with how it was originally constructed and can have hard-to-debug side effects for any later use of the same model/topo (including potentially V1/uh1). Prefer building a second model/topology with the desired face-to-cell ordering (e.g., via an UnstructuredDiscreteModel with permuted connectivity) instead of in-place mutation of cached topology data.

Suggested change
# how cells sharing a facet are listed in the model.
@testset "NormalSignMap" begin
# Create domain
domain = (0,1,0,1)
cells = (2,1)
model = CartesianDiscreteModel(domain,cells)
order = 0
reffe = ReferenceFE(raviart_thomas,Float64,order)
V1 = TestFESpace(model,reffe)
uh1 = FEFunction(V1,ones(num_free_dofs(V1)))
topo = get_grid_topology(model)
facet_cells = get_faces(topo,1,2)
interior_facets = 0
for facet in 1:(length(facet_cells.ptrs)-1)
first_cell = facet_cells.ptrs[facet]
last_cell = facet_cells.ptrs[facet+1] - 1
if last_cell - first_cell + 1 == 2
facet_cells.data[first_cell], facet_cells.data[last_cell] =
facet_cells.data[last_cell], facet_cells.data[first_cell]
interior_facets += 1
end
end
@test interior_facets == 1
V2 = TestFESpace(model,reffe)
# how cells sharing a facet are listed in the model.
function _build_two_cell_quad_model(cell_to_nodes)
node_coords = Point[
Point(0.0,0.0),
Point(0.5,0.0),
Point(1.0,0.0),
Point(0.0,1.0),
Point(0.5,1.0),
Point(1.0,1.0),
]
cell_reffe = ReferenceFE(lagrangian,Float64,1)
return UnstructuredDiscreteModel(node_coords,cell_to_nodes,cell_reffe)
end
@testset "NormalSignMap" begin
# Create two equivalent models whose cells are listed in opposite order.
# This exercises facet-to-cell ordering without mutating cached topology.
cell_to_nodes_1 = [
[1,2,5,4],
[2,3,6,5],
]
cell_to_nodes_2 = [
[2,3,6,5],
[1,2,5,4],
]
model1 = _build_two_cell_quad_model(cell_to_nodes_1)
model2 = _build_two_cell_quad_model(cell_to_nodes_2)
order = 0
reffe = ReferenceFE(raviart_thomas,Float64,order)
V1 = TestFESpace(model1,reffe)
uh1 = FEFunction(V1,ones(num_free_dofs(V1)))
V2 = TestFESpace(model2,reffe)

Copilot uses AI. Check for mistakes.
uh2 = FEFunction(V2,get_free_dof_values(uh1))

data_uh1 = get_data(uh1)
data_uh2 = get_data(uh2)

@test_broken evaluate(data_uh1[1],[Point(0.5,0.5)])[1] ≈ evaluate(data_uh2[1],[Point(0.5,0.5)])[1]
@test_broken evaluate(data_uh1[2],[Point(0.5,0.5)])[1] ≈ evaluate(data_uh2[2],[Point(0.5,0.5)])[1]
end

end # module
Loading