v0.1.2#237
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds MFEM 2D/3D export logic and tests, updates CI and pre-commit tooling, bumps package version to 0.1.2, introduces multiple MFEM test data files and new/updated test fixtures, and adds a PR template. Changes
Sequence Diagram(s)sequenceDiagram
participant T as test_mfem_export
participant E as gustaf.io.mfem.export
participant X2 as _export_2d
participant X3 as _export_3d
participant F as filesystem
T->>E: export(path, mesh)
E->>E: inspect mesh.vertices → dim
alt dim == 2
E->>X2: _export_2d(mesh)
X2->>X2: build elements, boundaries, vertices strings
X2-->>E: return component strings
else dim == 3
E->>X3: _export_3d(mesh)
X3->>X3: build elements, faces, vertices strings
X3-->>E: return component strings
else unsupported
E-->>T: raise NotImplementedError
end
E->>F: write MFEM header + composed sections
T->>F: read generated file
T->>T: compare with ground-truth using are_stripped_lines_same()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
.github/pull_request_template.md(1 hunks).github/workflows/tests.yml(1 hunks).pre-commit-config.yaml(3 hunks)gustaf/_version.py(1 hunks)gustaf/create/faces.py(1 hunks)gustaf/io/mfem.py(1 hunks)gustaf/utils/tictoc.py(1 hunks)pyproject.toml(2 hunks)tests/conftest.py(5 hunks)tests/data/mfem_hexahedra_3d.mesh(1 hunks)tests/data/mfem_hexahedra_3d_222.mesh(1 hunks)tests/data/mfem_quadrilaterals_2d.mesh(1 hunks)tests/data/mfem_tetrahedra_3d.mesh(1 hunks)tests/data/mfem_triangles_2d.mesh(1 hunks)tests/test_export.py(1 hunks)tests/test_vertices_and_element_updates.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-11T07:01:29.307Z
Learnt from: clemens-fricke
Repo: tataratat/gustaf PR: 215
File: gustaf/io/mfem.py:227-227
Timestamp: 2024-10-11T07:01:29.307Z
Learning: When no boundary conditions are provided in the `export` function of `gustaf/io/mfem.py`, add a comment to explain that a default boundary condition is being generated for all faces.
Applied to files:
tests/test_export.pygustaf/io/mfem.py
📚 Learning: 2024-07-03T08:27:40.999Z
Learnt from: clemens-fricke
Repo: tataratat/gustaf PR: 214
File: examples/export_meshio.py:52-52
Timestamp: 2024-07-03T08:27:40.999Z
Learning: The `export` function in the `gus.io.meshio` module accepts arguments in the order `(export_path, mesh, submeshes=None, **kwargs)`.
Applied to files:
gustaf/io/mfem.py
🧬 Code graph analysis (3)
tests/test_export.py (5)
tests/conftest.py (3)
to_tmpf(340-345)are_stripped_lines_same(245-336)vertices(54-55)gustaf/faces.py (3)
to_edges(302-317)single_faces(282-296)const_faces(229-240)gustaf/edges.py (2)
single_edges(221-235)const_edges(152-163)gustaf/volumes.py (1)
to_faces(266-281)gustaf/io/mfem.py (1)
export(126-159)
gustaf/io/mfem.py (3)
gustaf/vertices.py (3)
vertices(127-139)vertices(142-175)whatami(229-241)gustaf/faces.py (4)
whatami(141-152)edges(124-138)faces(183-195)faces(198-226)gustaf/volumes.py (1)
faces(118-136)
tests/conftest.py (2)
gustaf/faces.py (1)
Faces(72-338)gustaf/volumes.py (1)
Volumes(75-281)
🪛 Ruff (0.14.3)
gustaf/io/mfem.py
162-162: Missing return type annotation for private function _export_2d
(ANN202)
220-220: Missing return type annotation for private function _export_3d
(ANN202)
tests/conftest.py
246-246: Missing return type annotation for private function _are_stripped_lines_same
(ANN202)
246-246: Boolean default positional argument in function definition
(FBT002)
304-304: Do not catch blind exception: BaseException
(BLE001)
341-341: Missing return type annotation for private function _to_tmpf
(ANN202)
🔇 Additional comments (27)
.github/pull_request_template.md (1)
1-24: LGTM! Well-structured PR template.The template provides clear sections and checklists to guide contributors in documenting their changes effectively.
pyproject.toml (2)
54-65: LGTM! Dev dependencies and linting configuration updated appropriately.The addition of
pre-commitand reordering of dependencies align well with the pre-commit configuration updates in this PR. The Ruff target-version update topy39is consistent with the project's minimum Python version requirement.
83-86: LGTM! Ruff configuration appropriately excludes test data.Excluding
tests/datafrom linting is appropriate since these are data files, not code..pre-commit-config.yaml (3)
10-10: LGTM! Autoupdate branch configuration added.Setting the autoupdate branch to
developensures pre-commit CI updates are applied to the correct branch.
30-39: LGTM! Ruff configuration modernized.The upgrade to Ruff v0.14.3 and the introduction of a dedicated
ruff-formathook (replacing Black) consolidate formatting and linting under a single tool, which is a sensible improvement.
49-53: LGTM! Blackdoc version updated.The version bump from v0.4.1 to v0.4.5 keeps the tooling up-to-date.
gustaf/io/mfem.py (2)
110-123: LGTM! Simple and effective array formatter.The helper correctly converts NumPy arrays to MFEM's string format (space-separated values, newline-separated rows).
126-160: LGTM! Clean dimension-based dispatch.The export function appropriately determines the mesh dimension and delegates to specialized 2D/3D exporters. The file writing logic is clean and follows the MFEM format specification.
tests/data/mfem_hexahedra_3d_222.mesh (1)
1-73: LGTM! Valid MFEM mesh data for 2×2×2 hexahedral grid.The test data file correctly represents a 3D hexahedral mesh in MFEM v1.0 format with proper element connectivity, boundary markers, and vertex coordinates.
tests/data/mfem_hexahedra_3d.mesh (1)
1-29: LGTM! Valid MFEM mesh data for single hexahedral element.The test data file correctly represents a minimal 3D unit cube mesh in MFEM v1.0 format.
tests/data/mfem_quadrilaterals_2d.mesh (1)
1-23: LGTM! Valid MFEM mesh data for single quadrilateral element.The test data file correctly represents a minimal 2D unit square mesh in MFEM v1.0 format.
tests/test_vertices_and_element_updates.py (1)
14-17: LGTM! Test fixture update aligns with new test data.The change from
volumes_hexa333tovolumes_hexa_222correctly references the new fixture that corresponds to the added test data files.tests/data/mfem_tetrahedra_3d.mesh (1)
1-40: LGTM! Ground-truth test data is well-formed.The MFEM mesh file correctly represents a tetrahedral decomposition of a unit cube with proper header, dimension specification, element connectivity, boundary faces, and vertex coordinates.
tests/test_export.py (5)
1-15: LGTM! Test structure and parametrization are well-organized.The test parameters comprehensively cover 2D triangle/quad meshes and 3D tetrahedral/hexahedral meshes, with appropriate ground-truth file mappings.
18-29: LGTM! Test setup is correct.The parametrized test properly retrieves mesh fixtures dynamically and extracts vertices for boundary condition assignment.
30-42: LGTM! 2D boundary condition assignment is well-designed.The logic correctly identifies boundary edges and assigns different BC values based on x-coordinate thresholds, providing meaningful test coverage for MFEM export validation.
43-58: LGTM! 3D boundary condition assignment is consistent.The logic appropriately extends the 2D approach to 3D by assigning BC values to boundary faces based on x-coordinate thresholds, maintaining consistency across dimensions.
62-78: LGTM! Test execution and validation are properly structured.The test correctly exports the mesh and compares against ground-truth files. Note that
ignore_order=Truemeans word order within each line is ignored during comparison, but line-to-line correspondence is preserved.tests/conftest.py (9)
6-8: LGTM! Necessary imports for new utilities.The added imports support the new comparison and file handling utilities introduced in this file.
39-50: LGTM! 2D vertex fixture is correctly defined.The fixture properly creates a 2D unit square vertex array with appropriate dtype.
107-116: LGTM! 2D triangle connectivity is correct.The two triangles properly tessellate the unit square defined by vertices_2d.
124-126: LGTM! Fixture properly combines 2D vertices and triangle connectivity.
145-153: LGTM! 2D quadrilateral connectivity is correct.The single quad properly defines the unit square.
161-163: LGTM! Fixture properly combines 2D vertices and quad connectivity.
198-241: LGTM! 3D hexahedral grid fixture is correctly defined.The fixture properly constructs a 2×2×2 subdivision of a unit cube with 27 vertices and 8 hexahedral elements in standard connectivity order.
244-336: Comparison utility is well-designed for mesh file validation.The function provides robust line-by-line comparison with optional word-order insensitivity and numeric tolerance via
np.isclose, making it suitable for validating exported mesh files against ground truth.
339-345: LGTM! Simple utility for generating test file paths.The fixture provides a straightforward way to generate consistent temporary filenames for testing.
danielwolff1
left a comment
danielwolff1
left a comment
There was a problem hiding this comment.
lgtm, let's get this merged! 🚀
for more information, see https://pre-commit.ci
Co-authored-by: clemens.fricke <clemens.fricke@googlemail.com>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
clemens-fricke
force-pushed
the
develop
branch
from
9487cce to
dce1aca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/data/mfem_triangles_2d.mesh (1)
1-24: LGTM!The MFEM mesh file format is correct for MFEM v1.0, which uses 0-based indexing as confirmed by the MFEM specification. The mesh describes two triangular elements forming a unit square with proper connectivity and boundary definitions.
tests/conftest.py (1)
304-304: Replace overly broad exception handler.Catching
BaseExceptionat line 304 suppresses system-level exceptions likeKeyboardInterruptandSystemExit. Replace with more specific exception types such as(ValueError, LookupError)or justException.Apply this diff:
- except BaseException: + except (ValueError, LookupError):gustaf/io/mfem.py (2)
162-217: Implementation is correct; consider optional documentation improvements.The 2D export logic correctly handles triangle and quadrilateral meshes, builds appropriate element and boundary arrays, and formats vertices. The boundary construction safely handles
mesh.BCbecause it's initialized as an empty dictionary in the Faces class.As noted in past review comments, optional improvements include:
- Adding a return type annotation
-> tuple[str, str, str]for documentation- Adding a comment clarifying that when
mesh.BCis empty, a zero-length boundary section is producedBased on learnings
220-282: Implementation is correct; consider optional documentation improvements.The 3D export logic correctly handles tetrahedral and hexahedral meshes, appropriately determines geometry types for body and face elements, and builds the required arrays. Like the 2D exporter, boundary handling is safe due to BC initialization in the base class.
The same optional improvements suggested for
_export_2dapply here:
- Adding a return type annotation
-> tuple[str, str, str]for consistency- Adding a comment about empty BC behavior
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
.github/pull_request_template.md(1 hunks).github/workflows/tests.yml(1 hunks).pre-commit-config.yaml(3 hunks)gustaf/_version.py(1 hunks)gustaf/create/faces.py(1 hunks)gustaf/io/mfem.py(1 hunks)gustaf/utils/tictoc.py(1 hunks)pyproject.toml(2 hunks)tests/conftest.py(5 hunks)tests/data/mfem_hexahedra_3d.mesh(1 hunks)tests/data/mfem_hexahedra_3d_222.mesh(1 hunks)tests/data/mfem_quadrilaterals_2d.mesh(1 hunks)tests/data/mfem_tetrahedra_3d.mesh(1 hunks)tests/data/mfem_triangles_2d.mesh(1 hunks)tests/test_export.py(1 hunks)tests/test_vertices_and_element_updates.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-11T07:01:29.307Z
Learnt from: clemens-fricke
Repo: tataratat/gustaf PR: 215
File: gustaf/io/mfem.py:227-227
Timestamp: 2024-10-11T07:01:29.307Z
Learning: When no boundary conditions are provided in the `export` function of `gustaf/io/mfem.py`, add a comment to explain that a default boundary condition is being generated for all faces.
Applied to files:
tests/test_export.pygustaf/io/mfem.py
📚 Learning: 2024-07-03T08:27:40.999Z
Learnt from: clemens-fricke
Repo: tataratat/gustaf PR: 214
File: examples/export_meshio.py:52-52
Timestamp: 2024-07-03T08:27:40.999Z
Learning: The `export` function in the `gus.io.meshio` module accepts arguments in the order `(export_path, mesh, submeshes=None, **kwargs)`.
Applied to files:
gustaf/io/mfem.py
🧬 Code graph analysis (3)
tests/test_export.py (5)
tests/conftest.py (3)
to_tmpf(340-345)are_stripped_lines_same(245-336)vertices(54-55)gustaf/faces.py (3)
to_edges(302-317)single_faces(282-296)const_faces(229-240)gustaf/edges.py (2)
single_edges(221-235)const_edges(152-163)gustaf/volumes.py (1)
to_faces(266-281)gustaf/io/mfem.py (1)
export(126-159)
tests/conftest.py (2)
gustaf/faces.py (1)
Faces(72-338)gustaf/volumes.py (1)
Volumes(75-281)
gustaf/io/mfem.py (5)
gustaf/io/meshio.py (1)
export(87-189)gustaf/vertices.py (3)
vertices(127-139)vertices(142-175)whatami(229-241)gustaf/edges.py (5)
elements(238-255)elements(258-274)whatami(166-177)edges(111-123)edges(126-149)gustaf/faces.py (4)
whatami(141-152)edges(124-138)faces(183-195)faces(198-226)gustaf/volumes.py (1)
faces(118-136)
🪛 Ruff (0.14.4)
tests/conftest.py
246-246: Missing return type annotation for private function _are_stripped_lines_same
(ANN202)
246-246: Boolean default positional argument in function definition
(FBT002)
304-304: Do not catch blind exception: BaseException
(BLE001)
341-341: Missing return type annotation for private function _to_tmpf
(ANN202)
gustaf/io/mfem.py
162-162: Missing return type annotation for private function _export_2d
(ANN202)
220-220: Missing return type annotation for private function _export_3d
(ANN202)
🔇 Additional comments (23)
gustaf/create/faces.py (1)
127-129: Logging message clarity is fine.Single literal keeps the debug message identical without changing behavior.
gustaf/utils/tictoc.py (1)
91-92: Cumulative formatting matches elapsed time.Computing
lap - startensures the summary shows the expected relative values.gustaf/_version.py (1)
6-6: Version constant updated appropriately.The module version now matches the v0.1.2 release target.
.github/pull_request_template.md (1)
1-24: Template structure looks solid.The added sections and checklist should help standardize incoming PRs.
pyproject.toml (2)
55-65: Dev extras update looks good.Including
pre-commitalongside the existing tooling keeps the developer environment complete.
85-87: Ruff configuration aligns with Python requirement.Setting the target to py39 and excluding
tests/datamatches the declaredrequires-python >=3.9and avoids lint noise from fixtures..pre-commit-config.yaml (2)
10-10: LGTM!Setting
autoupdate_branchtodevelopensures that pre-commit.ci auto-updates target the correct branch for your workflow.
50-53: LGTM!The blackdoc version bump from v0.4.1 to v0.4.5 is a routine maintenance update with no functional changes to the configuration.
.github/workflows/tests.yml (1)
14-14: LGTM!Removing
macos-13from the matrix in favor ofmacos-latestis appropriate, as GitHub Actions is transitioning away from Intel-based macOS runners. The matrix still provides good coverage across Ubuntu, macOS, and Windows.tests/data/mfem_hexahedra_3d.mesh (1)
1-29: LGTM!The 3D hexahedral MFEM mesh file is correctly formatted with one hex element (geometry type 5), six boundary faces, and eight vertices forming a unit cube. The connectivity and vertex coordinates are consistent with MFEM v1.0 specifications.
tests/data/mfem_hexahedra_3d_222.mesh (1)
1-73: LGTM!The 3D hexahedral mesh with 2×2×2 topology is correctly formatted. It contains 8 hex elements, 27 vertices (3×3×3 grid points), and 24 boundary entries representing the exterior faces. The vertex coordinates progress in 0.5 increments, forming a properly structured grid.
tests/data/mfem_quadrilaterals_2d.mesh (1)
1-23: LGTM!The 2D quadrilateral MFEM mesh file is correctly formatted with one quad element (geometry type 3), four boundary edges, and four vertices forming a unit square. The format conforms to MFEM v1.0 specifications.
tests/data/mfem_tetrahedra_3d.mesh (1)
1-40: LGTM!The 3D tetrahedral MFEM mesh file is correctly formatted with six tetrahedral elements (geometry type 4) decomposing a unit cube, twelve boundary triangular faces, and eight vertices. The mesh structure is valid for MFEM v1.0.
tests/test_vertices_and_element_updates.py (1)
16-16: Fixture migration verified successfully.The new
volumes_hexa_222fixture is properly defined and the oldvolumes_hexa333fixture has been completely removed with no remaining references. The change is correctly applied throughout the test suite.tests/test_export.py (4)
1-15: LGTM!The imports are appropriate, and the parametrization setup correctly maps test fixtures to their corresponding ground truth files.
28-42: LGTM!The 2D boundary condition assignment logic is clear and correct. Using
single_edges()to identify boundary edges and coordinate-based BC assignment is appropriate for testing.
43-58: LGTM!The 3D boundary condition assignment correctly mirrors the 2D approach, using
single_faces()to identify boundary faces and applying the same coordinate-based BC logic.
60-78: LGTM!The export and comparison logic is well-structured. Using a temporary directory for isolation and comparing with
ignore_order=Trueis appropriate for validating MFEM format exports.gustaf/io/mfem.py (2)
110-123: LGTM!The
format_arrayutility function correctly formats NumPy arrays into space-separated, line-delimited strings suitable for the MFEM format.
126-159: LGTM!The
exportfunction correctly dispatches to dimension-specific exporters and writes the MFEM v1.0 format with proper structure. The dimension detection and error handling for unsupported dimensions are appropriate.tests/conftest.py (3)
39-50: LGTM!The new 2D fixtures (
vertices_2d,tri_connec_2d,faces_tri_2d,quad_connec_2d,faces_quad_2d) are correctly structured and consistent with their 3D counterparts. These fixtures appropriately support the MFEM 2D export tests.Also applies to: 107-116, 124-126, 145-153, 161-163
198-241: LGTM!The
volumes_hexa_222fixture correctly creates a 2×2×2 structured hexahedral mesh with 27 vertices and 8 elements. This provides a more complex test case for 3D export validation.
339-345: LGTM!The
to_tmpffixture correctly generates a temporary file path within a given directory. The hardcoded filename is sufficient for test isolation.
4 participants
New patch version introducing mfem 3D export. Thanks for the work on this @mkofler96.
Summary by CodeRabbit
New Features
Chores
Tests
Documentation