Adding a couple of broken tests to DivConformingFESpacesTests#1292
Adding a couple of broken tests to DivConformingFESpacesTests#1292amartinhuertas wants to merge 4 commits into
Conversation
These will be sorted out soon in another branch.
There was a problem hiding this comment.
Pull request overview
Adds a regression test (currently marked @test_broken) intended to expose order-dependence in NormalSignMap facet-owner selection, and records the addition in the changelog.
Changes:
- Added a new
@testset "NormalSignMap"to reproduce/signpost a facet-owner ordering issue via@test_brokenassertions. - Added an “Unreleased / Added” entry to
NEWS.mddocumenting the new broken tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
test/FESpacesTests/DivConformingFESpacesTests.jl |
Introduces a new NormalSignMap broken test by mutating facet-to-cell adjacency ordering and comparing resulting evaluations. |
NEWS.md |
Documents the addition of the broken tests under the Unreleased section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1292 +/- ##
==========================================
- Coverage 88.83% 88.26% -0.58%
==========================================
Files 227 227
Lines 29769 29769
==========================================
- Hits 26445 26275 -170
- Misses 3324 3494 +170
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 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) |
There was a problem hiding this comment.
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.
| # 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) |
| # 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. |
There was a problem hiding this comment.
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.
| # 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. |
| ## [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) |
There was a problem hiding this comment.
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]).
| - 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). |
|
We will merge the changes in this branch through a different PR. In particular: #1291 Closing this PR ... |
No description provided.