added MFEM 3D export feature #215
Conversation
WalkthroughThe update enhances the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
This looks very nice. The automatic code adaptations seem to be straightforward and especially the documentation remarks are good. In addition to that, could you also add the example you have given up top to the test suite and the examples? Thanks for your work. |
|
Now that you have added the BC keyword to the function call, we need to think about how we need to include it for the other cases as well. Currently it is only implemented for the 3D case, which might produce unwanted/confusing behavior for users. @j042 what is the status of the BC object variable? Could we not use that instead? I have no idea what the status of general BC in gustaf is, but I know that we talked about it previously... |
We talked about having general helper class to handle subset of elements/subelements - no progress on that. However, Faces and Volumes have member called |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (4)
- gustaf/io/mfem.py (2 hunks)
- tests/conftest.py (2 hunks)
- tests/data/mfem_tetrahedra_3d.mesh (1 hunks)
- tests/test_export.py (1 hunks)
Additional context used
Ruff
tests/conftest.py
172-172: Line too long (83 > 79)
(E501)
227-227: Line too long (81 > 79)
(E501)
Additional comments not posted (10)
tests/data/mfem_tetrahedra_3d.mesh (3)
1-4: Header and Dimension Section: Looks Good!The header and dimension are correctly defined and follow the MFEM mesh format.
6-13: Elements Section: Looks Good!The elements are correctly defined and consistent with the 3D dimension.
15-40: Boundary and Vertices Sections: Looks Good!The boundary faces and vertices are correctly defined and consistent with the elements.
tests/test_export.py (3)
1-27: Imports and Mesh Definition: Looks Good!The necessary modules are imported, and the 3D tetrahedral mesh is correctly defined.
29-47: Boundary Condition Assignment Logic: Looks Good!The boundary conditions are correctly assigned based on the x-coordinate of the vertices.
49-61: Export and Comparison Logic: Looks Good!The mesh is correctly exported to a temporary file and compared with a reference file using the custom fixture.
tests/conftest.py (2)
170-233: Fixtureare_stripped_lines_same: Looks Good!The fixture correctly compares lines while ignoring whitespace differences. The logic is well-implemented and should handle the comparison as expected.
Tools
Ruff
172-172: Line too long (83 > 79)
(E501)
227-227: Line too long (81 > 79)
(E501)
237-242: Fixtureto_tmpf: Looks Good!The fixture correctly converts a temporary directory to a temporary file path. The logic is well-implemented and should handle the conversion as expected.
gustaf/io/mfem.py (2)
Line range hint
1-106: Functionload: Looks Good!The
loadfunction is correctly implemented and consistent with the export function.
120-123: Clarify theBCparameter in the docstring.Mention that
BCis optional and explain the default behavior if not provided.- BC: dict - Boundary conditions for 3D case. - Keys are boundary IDs and values are lists of face IDs. - If none are provided, all boundary faces are assigned to 1. + BC: dict, optional + Boundary conditions for 3D case. Keys are boundary IDs and values + are lists of face IDs. If not provided, all boundary faces are + assigned to 1.Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- tests/conftest.py (2 hunks)
Additional context used
Ruff
tests/conftest.py
227-227: Line too long (81 > 79)
(E501)
Additional comments not posted (1)
tests/conftest.py (1)
201-201: Enhance print statements for better debugging.The print statement can be made more informative by providing additional context.
- print(f"\t\tdifferent word counts: a-{len_a}, b-{len_b}") + print(f"\t\tDifferent word counts: a-{len_a}, b-{len_b}")Likely invalid or redundant comment.
| def _to_tmpf(tmpd): | ||
| """given tmpd, returns tmpf""" | ||
| return os.path.join(tmpd, "nqv248p90") |
There was a problem hiding this comment.
Improve the docstring for clarity.
The current docstring does not clearly explain the function's behavior and parameters.
- """given tmpd, returns tmpf"""
+ """
+ Given a temporary directory path `tmpd`, returns a temporary file path `tmpf`.
+ """Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _to_tmpf(tmpd): | |
| """given tmpd, returns tmpf""" | |
| return os.path.join(tmpd, "nqv248p90") | |
| def _to_tmpf(tmpd): | |
| """ | |
| Given a temporary directory path `tmpd`, returns a temporary file path `tmpf`. | |
| """ | |
| return os.path.join(tmpd, "nqv248p90") |
| if stripped_a != stripped_b and ignore_order: | ||
| print("\tchecking again, while ignoring word order:") | ||
|
|
There was a problem hiding this comment.
Enhance print statements for better debugging.
The print statement can be made more informative by providing additional context.
- print("\tchecking again, while ignoring word order:")
+ print("\tChecking again, while ignoring word order:")Committable suggestion was skipped due to low confidence.
tests/conftest.py
Outdated
| break | ||
| else: | ||
| print( | ||
| f"\t\tsecond does not contain ({word_a})" |
There was a problem hiding this comment.
Line too long.
The line exceeds the maximum line length of 79 characters, making it harder to read.
- f"\t\tsecond does not contain ({word_a})"
+ f"\t\tSecond does not contain ({word_a})"Committable suggestion was skipped due to low confidence.
Tools
Ruff
227-227: Line too long (81 > 79)
(E501)
| print(f"stripped line at index-{i} are not the same") | ||
| print(f"\tfrom first: {line_a}") | ||
| print(f"\tfrom second: {line_b}") |
There was a problem hiding this comment.
Enhance print statements for better debugging.
The print statements can be made more informative by providing additional context.
- print(f"stripped line at index-{i} are not the same")
- print(f"\tfrom first: {line_a}")
- print(f"\tfrom second: {line_b}")
+ print(f"Stripped lines at index {i} are not the same:")
+ print(f"\tFrom first: {line_a}")
+ print(f"\tFrom second: {line_b}")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print(f"stripped line at index-{i} are not the same") | |
| print(f"\tfrom first: {line_a}") | |
| print(f"\tfrom second: {line_b}") | |
| print(f"Stripped lines at index {i} are not the same:") | |
| print(f"\tFrom first: {line_a}") | |
| print(f"\tFrom second: {line_b}") |
| def are_stripped_lines_same(): | ||
| def _are_stripped_lines_same(a, b, ignore_order=False): | ||
| """returns True if items in a and b same, preceding and tailing | ||
| whitespaces are ignored and strings are joined""" |
There was a problem hiding this comment.
Improve the docstring for clarity.
The current docstring does not clearly explain the function's behavior and parameters.
- """returns True if items in a and b same, preceding and tailing
- whitespaces are ignored and strings are joined"""
+ """
+ Returns True if items in `a` and `b` are the same after stripping preceding
+ and trailing whitespaces. If `ignore_order` is True, checks if the words
+ in the lines match in any order.
+ """Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def are_stripped_lines_same(): | |
| def _are_stripped_lines_same(a, b, ignore_order=False): | |
| """returns True if items in a and b same, preceding and tailing | |
| whitespaces are ignored and strings are joined""" | |
| def are_stripped_lines_same(): | |
| def _are_stripped_lines_same(a, b, ignore_order=False): | |
| """ | |
| Returns True if items in `a` and `b` are the same after stripping preceding | |
| and trailing whitespaces. If `ignore_order` is True, checks if the words | |
| in the lines match in any order. | |
| """ |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (4)
- gustaf/io/mfem.py (2 hunks)
- tests/conftest.py (2 hunks)
- tests/data/mfem_hexahedra_3d.mesh (1 hunks)
- tests/test_export.py (1 hunks)
Additional context used
Learnings (1)
gustaf/io/mfem.py (1)
Learnt from: clemens-fricke PR: tataratat/gustaf#215 File: gustaf/io/mfem.py:227-227 Timestamp: 2024-07-10T07:33:22.062Z 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.
Additional comments not posted (15)
tests/data/mfem_hexahedra_3d.mesh (4)
1-4: Correctly specified MFEM mesh version and dimension.The mesh version is specified as
v1.0and the dimension is correctly set to3.
6-15: Elements section is correctly formatted.The elements section specifies 8 elements, each with 10 values. The format follows MFEM specifications.
17-42: Boundary section is correctly formatted.The boundary section specifies 24 boundary elements, each with 6 values. The format follows MFEM specifications.
44-73: Vertices section is correctly formatted.The vertices section specifies 27 vertices, each with 3 coordinates. The format follows MFEM specifications.
tests/test_export.py (4)
1-8: Necessary imports for testing are present.The imports include standard libraries,
numpy,pytest, and thegustaflibrary, which are necessary for the tests.
9-12: Comprehensive list of grids for parameterized tests.The
all_gridstuple includes two grid types, ensuring comprehensive test coverage for hexahedral and tetrahedral meshes.
15-35: Fixture for tetrahedral volumes is correctly defined.The
volumes_tetrafixture defines vertices and volumes for a tetrahedral mesh, returning agus.Volumesobject.
38-73: Parameterized test function for MFEM export is comprehensive.The
test_mfem_exportfunction uses theall_gridsparameter, retrieves the mesh fixture, and performs export tests with and without boundary conditions. The test ensures that the exported file matches the ground truth.gustaf/io/mfem.py (7)
112-114: Enhance docstring to reflect new functionality.The docstring should mention the support for boundary conditions explicitly.
- """Export mesh in MFEM format. Supports 2D triangle and quadrilateral - meshes as well as 3D tetrahedral meshes. Does not support different - element attributes or difference in vertex dimension and mesh dimension. + """Export mesh in MFEM format. Supports 2D triangle and quadrilateral + meshes as well as 3D tetrahedral meshes. Handles boundary conditions for + 3D tetrahedral meshes. Does not support different element attributes or + difference in vertex dimension and mesh dimension.
120-123: Clarify theBCparameter in the docstring.Mention that
BCis optional and explain the default behavior if not provided.- BC: dict - Boundary conditions for 3D case. - Keys are boundary IDs and values are lists of face IDs. - If none are provided, all boundary faces are assigned to 1. + BC: dict, optional + Boundary conditions for 3D case. Keys are boundary IDs and values + are lists of face IDs. If not provided, all boundary faces are + assigned to 1.
206-218: EnsureBCis a dictionary.Before using
BC, ensure it is a dictionary to avoid potential errors.+ if not isinstance(BC, dict): + raise TypeError("BC must be a dictionary with boundary IDs as keys and lists of face IDs.")
231-234: Check for emptyfaceidslist.Ensure that
faceidsis not empty before processing to avoid potential errors.+ if not faceids: + continue
240-241: Check for emptyfaceidslist.Ensure that
faceidsis not empty before processing to avoid potential errors.+ if not faceids: + continue
227-227: Add a comment to explain the default boundary condition.When no boundary conditions are provided, add a comment to explain that a default boundary condition is being generated for all faces.
# No boundary conditions provided; generating one for all faces BC = {1: mesh.to_faces(False).single_faces()}
253-258: Ensure vertices are correctly exported for 3D mesh.The changes for exporting vertices for 3D mesh look correct and efficient.
| @pytest.fixture | ||
| def are_stripped_lines_same(): | ||
| def _are_stripped_lines_same(a, b, ignore_order=False): | ||
| """returns True if items in a and b same, preceding and tailing | ||
| whitespaces are ignored and strings are joined""" | ||
| all_same = True | ||
|
|
||
| for i, (line_a, line_b) in enumerate(zip(a, b)): | ||
| # check stripped string | ||
| stripped_a, stripped_b = line_a.strip(), line_b.strip() | ||
|
|
||
| # print general info | ||
| if stripped_a != stripped_b: | ||
| print(f"stripped line at index-{i} are not the same") | ||
| print(f"\tfrom first: {line_a}") | ||
| print(f"\tfrom second: {line_b}") | ||
|
|
||
| # give one more chance if ignore_order | ||
| if stripped_a != stripped_b and ignore_order: | ||
| print("\tchecking again, while ignoring word order:") | ||
|
|
||
| # This is meant for attributes | ||
| delimiters = r" |\>|\<|\t|," | ||
| splitted_a = list( | ||
| filter(None, re.split(delimiters, stripped_a)) | ||
| ) | ||
| splitted_b = list( | ||
| filter(None, re.split(delimiters, stripped_b)) | ||
| ) | ||
| # first, len check | ||
| len_a, len_b = len(splitted_a), len(splitted_b) | ||
| if len(splitted_a) != len(splitted_b): | ||
| print(f"\t\tdifferent word counts: a-{len_a}, b-{len_b}") | ||
| all_same = False | ||
| else: | ||
| # word order | ||
| a_to_b = [] | ||
| nums_b = None | ||
| for word_a in splitted_a: | ||
| try: | ||
| a_to_b.append(splitted_b.index(word_a)) | ||
| except BaseException: | ||
| try: | ||
| num_a = float(word_a) | ||
| except ValueError: | ||
| pass | ||
| else: | ||
| if nums_b is None: | ||
| nums_b = [] | ||
| for idx, num_b in enumerate(splitted_b): | ||
| with contextlib.suppress(ValueError): | ||
| nums_b.append((idx, float(num_b))) | ||
| for idx, num_b in nums_b: | ||
| if np.isclose(num_a, num_b): | ||
| a_to_b.append(idx) | ||
| break | ||
| else: | ||
| print( | ||
| f"\t\tsecond does not contain " | ||
| f"({word_a})" | ||
| ) | ||
| all_same = False | ||
|
|
||
| return all_same | ||
|
|
There was a problem hiding this comment.
Fixture for comparing stripped lines is comprehensive but could use clearer docstrings.
The are_stripped_lines_same fixture is well-defined, providing detailed comparison logic for lists of strings, including an option to ignore word order. However, the docstring can be improved for clarity.
- """returns True if items in a and b same, preceding and tailing
- whitespaces are ignored and strings are joined"""
+ """
+ Returns True if items in `a` and `b` are the same after stripping preceding
+ and trailing whitespaces. If `ignore_order` is True, checks if the words
+ in the lines match in any order.
+ """Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.fixture | |
| def are_stripped_lines_same(): | |
| def _are_stripped_lines_same(a, b, ignore_order=False): | |
| """returns True if items in a and b same, preceding and tailing | |
| whitespaces are ignored and strings are joined""" | |
| all_same = True | |
| for i, (line_a, line_b) in enumerate(zip(a, b)): | |
| # check stripped string | |
| stripped_a, stripped_b = line_a.strip(), line_b.strip() | |
| # print general info | |
| if stripped_a != stripped_b: | |
| print(f"stripped line at index-{i} are not the same") | |
| print(f"\tfrom first: {line_a}") | |
| print(f"\tfrom second: {line_b}") | |
| # give one more chance if ignore_order | |
| if stripped_a != stripped_b and ignore_order: | |
| print("\tchecking again, while ignoring word order:") | |
| # This is meant for attributes | |
| delimiters = r" |\>|\<|\t|," | |
| splitted_a = list( | |
| filter(None, re.split(delimiters, stripped_a)) | |
| ) | |
| splitted_b = list( | |
| filter(None, re.split(delimiters, stripped_b)) | |
| ) | |
| # first, len check | |
| len_a, len_b = len(splitted_a), len(splitted_b) | |
| if len(splitted_a) != len(splitted_b): | |
| print(f"\t\tdifferent word counts: a-{len_a}, b-{len_b}") | |
| all_same = False | |
| else: | |
| # word order | |
| a_to_b = [] | |
| nums_b = None | |
| for word_a in splitted_a: | |
| try: | |
| a_to_b.append(splitted_b.index(word_a)) | |
| except BaseException: | |
| try: | |
| num_a = float(word_a) | |
| except ValueError: | |
| pass | |
| else: | |
| if nums_b is None: | |
| nums_b = [] | |
| for idx, num_b in enumerate(splitted_b): | |
| with contextlib.suppress(ValueError): | |
| nums_b.append((idx, float(num_b))) | |
| for idx, num_b in nums_b: | |
| if np.isclose(num_a, num_b): | |
| a_to_b.append(idx) | |
| break | |
| else: | |
| print( | |
| f"\t\tsecond does not contain " | |
| f"({word_a})" | |
| ) | |
| all_same = False | |
| return all_same | |
| """ | |
| Returns True if items in `a` and `b` are the same after stripping preceding | |
| and trailing whitespaces. If `ignore_order` is True, checks if the words | |
| in the lines match in any order. | |
| """ |
| def to_tmpf(): | ||
| def _to_tmpf(tmpd): | ||
| """given tmpd, returns tmpf""" | ||
| return os.path.join(tmpd, "nqv248p90") | ||
|
|
||
| return _to_tmpf |
There was a problem hiding this comment.
Fixture for creating temporary file paths is well-defined but could use a clearer docstring.
The to_tmpf fixture simplifies creating temporary file paths. The docstring can be improved for clarity.
- """given tmpd, returns tmpf"""
+ """
+ Given a temporary directory path `tmpd`, returns a temporary file path `tmpf`.
+ """Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def to_tmpf(): | |
| def _to_tmpf(tmpd): | |
| """given tmpd, returns tmpf""" | |
| return os.path.join(tmpd, "nqv248p90") | |
| return _to_tmpf | |
| def to_tmpf(): | |
| def _to_tmpf(tmpd): | |
| """ | |
| Given a temporary directory path `tmpd`, returns a temporary file path `tmpf`. | |
| """ | |
| return os.path.join(tmpd, "nqv248p90") | |
| return _to_tmpf |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- tests/test_export.py (1 hunks)
Additional comments not posted (5)
tests/test_export.py (5)
1-8: LGTM! Imports are necessary and appropriate.The import statements are essential for the test functionality.
9-12: LGTM! Theall_gridsvariable is well-defined.The variable is used for parameterizing the test function and appears correct.
15-19: LGTM! The test function is well-defined and parameterized.The function uses fixtures and parameters appropriately.
20-37: LGTM! The mesh processing logic is correct.The function processes the mesh vertices, converts to faces, and identifies boundary conditions appropriately.
39-50: LGTM! The test output logic is correct.The function creates a temporary directory, exports the mesh, and compares the output with the ground truth appropriately.
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
j042
force-pushed
the
ft-mfem-3D-export
branch
from
1cf9b85 to
549e7b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (5)
- gustaf/io/mfem.py (2 hunks)
- tests/conftest.py (2 hunks)
- tests/data/mfem_hexahedra_3d.mesh (1 hunks)
- tests/data/mfem_tetrahedra_3d.mesh (1 hunks)
- tests/test_export.py (1 hunks)
Additional comments not posted (7)
tests/data/mfem_tetrahedra_3d.mesh (1)
1-40: MFEM Tetrahedral Mesh Format is Correct.The file is well-structured and follows the MFEM mesh format for a 3D tetrahedral mesh. No issues found.
tests/data/mfem_hexahedra_3d.mesh (1)
1-73: MFEM Hexahedral Mesh Format is Correct.The file is well-structured and follows the MFEM mesh format for a 3D hexahedral mesh. No issues found.
tests/conftest.py (2)
169-233: Improve the docstring and print statements inare_stripped_lines_same.The docstring and print statements can be improved for clarity and consistency.
- """returns True if items in a and b same, preceding and tailing - whitespaces are ignored and strings are joined""" + """ + Returns True if items in `a` and `b` are the same after stripping + preceding and trailing whitespaces. If `ignore_order` is True, + checks if the words in the lines match in any order. + """ - print(f"stripped line at index-{i} are not the same") - print(f"\tfrom first: {line_a}") - print(f"\tfrom second: {line_b}") + print(f"Stripped lines at index {i} are not the same:") + print(f"\tFrom first: {line_a}") + print(f"\tFrom second: {line_b}")
238-243: Improve the docstring forto_tmpf.The docstring can be made clearer to better describe the function's purpose.
- """given tmpd, returns tmpf""" + """ + Given a temporary directory path `tmpd`, returns a temporary file path `tmpf`. + """gustaf/io/mfem.py (3)
112-114: Enhance the docstring to reflect new functionality.The docstring should explicitly mention the support for boundary conditions in 3D tetrahedral meshes.
- """Export mesh in MFEM format. Supports 2D triangle and quadrilateral - meshes as well as 3D tetrahedral meshes. Does not support different - element attributes or difference in vertex dimension and mesh dimension. + """Export mesh in MFEM format. Supports 2D triangle and quadrilateral + meshes as well as 3D tetrahedral meshes. Handles boundary conditions for + 3D tetrahedral meshes. Does not support different element attributes or + difference in vertex dimension and mesh dimension.
120-123: Clarify theBCparameter in the docstring.Mention that
BCis optional and explain the default behavior if not provided.- BC: dict - Boundary conditions for 3D case. - Keys are boundary IDs and values are lists of face IDs. - If none are provided, all boundary faces are assigned to 1. + BC: dict, optional + Boundary conditions for 3D case. Keys are boundary IDs and values + are lists of face IDs. If not provided, all boundary faces are + assigned to 1.
206-264: EnsureBCis a dictionary and check for emptyfaceids.Before using
BC, ensure it is a dictionary to avoid potential errors. Also, ensure thatfaceidsis not empty before processing.+ if not isinstance(BC, dict): + raise TypeError("BC must be a dictionary with boundary IDs as keys and lists of face IDs as values.") # Boundary faces = mesh.faces() nboundary_faces = sum(map(len, mesh.BC.values())) boundary_array = np.empty( (nboundary_faces, n_face_vertices + 2), dtype=settings.INT_DTYPE ) startrow = 0 # Add boundary one by one as TRIANGLEs for bid, faceids in mesh.BC.items(): + if not faceids: + continue nfaces = len(faceids) e = np.ones(nfaces).reshape(-1, 1) vertex_list = faces[faceids, :] boundary_array[startrow : (startrow + nfaces), :] = np.hstack( (int(bid) * e, face_geometry_type * e, vertex_list) ) startrow += nfaces
tests/test_export.py
Outdated
| @pytest.mark.parametrize("grid", all_grids) | ||
| def test_mfem_export(to_tmpf, are_stripped_lines_same, grid, request): | ||
| mesh = request.getfixturevalue(grid[0]) | ||
| ground_truth_filename = grid[1] | ||
|
|
||
| verts = mesh.vertices | ||
|
|
||
| faces = mesh.to_faces(False) | ||
| boundary_faces = faces.single_faces() | ||
|
|
||
| BC = {1: [], 2: [], 3: []} | ||
| for i in boundary_faces: | ||
| # mark boundaries at x = 0 with 1 | ||
| if np.max(verts[faces.const_faces[i], 0]) < 0.1: | ||
| BC[1].append(i) | ||
| # mark boundaries at x = 1 with 2 | ||
| elif np.min(verts[faces.const_faces[i], 0]) > 0.9: | ||
| BC[2].append(i) | ||
| # mark rest of the boundaries with 3 | ||
| else: | ||
| BC[3].append(i) | ||
|
|
||
| mesh.BC = BC | ||
|
|
||
| # Test output | ||
| with tempfile.TemporaryDirectory() as tmpd: | ||
| tmpf = to_tmpf(tmpd) | ||
| gus.io.mfem.export(tmpf, mesh) | ||
|
|
||
| with open(tmpf) as tmp_read, open( | ||
| os.path.dirname(__file__) + f"/./data/{ground_truth_filename}" | ||
| ) as base_file: | ||
| print(base_file.readlines()) | ||
| assert are_stripped_lines_same( | ||
| base_file.readlines(), tmp_read.readlines(), True | ||
| ) |
There was a problem hiding this comment.
Enhance Test Readability and Debugging.
The test function is well-structured, but consider removing or commenting out the print statement used for debugging, as it may clutter the test output.
Apply this diff to remove the print statement:
- print(base_file.readlines())Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.parametrize("grid", all_grids) | |
| def test_mfem_export(to_tmpf, are_stripped_lines_same, grid, request): | |
| mesh = request.getfixturevalue(grid[0]) | |
| ground_truth_filename = grid[1] | |
| verts = mesh.vertices | |
| faces = mesh.to_faces(False) | |
| boundary_faces = faces.single_faces() | |
| BC = {1: [], 2: [], 3: []} | |
| for i in boundary_faces: | |
| # mark boundaries at x = 0 with 1 | |
| if np.max(verts[faces.const_faces[i], 0]) < 0.1: | |
| BC[1].append(i) | |
| # mark boundaries at x = 1 with 2 | |
| elif np.min(verts[faces.const_faces[i], 0]) > 0.9: | |
| BC[2].append(i) | |
| # mark rest of the boundaries with 3 | |
| else: | |
| BC[3].append(i) | |
| mesh.BC = BC | |
| # Test output | |
| with tempfile.TemporaryDirectory() as tmpd: | |
| tmpf = to_tmpf(tmpd) | |
| gus.io.mfem.export(tmpf, mesh) | |
| with open(tmpf) as tmp_read, open( | |
| os.path.dirname(__file__) + f"/./data/{ground_truth_filename}" | |
| ) as base_file: | |
| print(base_file.readlines()) | |
| assert are_stripped_lines_same( | |
| base_file.readlines(), tmp_read.readlines(), True | |
| ) | |
| @pytest.mark.parametrize("grid", all_grids) | |
| def test_mfem_export(to_tmpf, are_stripped_lines_same, grid, request): | |
| mesh = request.getfixturevalue(grid[0]) | |
| ground_truth_filename = grid[1] | |
| verts = mesh.vertices | |
| faces = mesh.to_faces(False) | |
| boundary_faces = faces.single_faces() | |
| BC = {1: [], 2: [], 3: []} | |
| for i in boundary_faces: | |
| # mark boundaries at x = 0 with 1 | |
| if np.max(verts[faces.const_faces[i], 0]) < 0.1: | |
| BC[1].append(i) | |
| # mark boundaries at x = 1 with 2 | |
| elif np.min(verts[faces.const_faces[i], 0]) > 0.9: | |
| BC[2].append(i) | |
| # mark rest of the boundaries with 3 | |
| else: | |
| BC[3].append(i) | |
| mesh.BC = BC | |
| # Test output | |
| with tempfile.TemporaryDirectory() as tmpd: | |
| tmpf = to_tmpf(tmpd) | |
| gus.io.mfem.export(tmpf, mesh) | |
| with open(tmpf) as tmp_read, open( | |
| os.path.dirname(__file__) + f"/./data/{ground_truth_filename}" | |
| ) as base_file: | |
| assert are_stripped_lines_same( | |
| base_file.readlines(), tmp_read.readlines(), True | |
| ) |
| @@ -160,3 +164,80 @@ def provide_data_to_unittest( | |||
| request.cls.QF = quad_connec | |||
| request.cls.TV = tet_connec | |||
| request.cls.HV = hexa_connec | |||
|
|
|||
|
|
|||
| @pytest.fixture | |||
| import gustaf as gus | ||
|
|
||
| all_grids = ( | ||
| ("volumes_hexa", "mfem_hexahedra_3d.mesh"), |
There was a problem hiding this comment.
I added it. It's a bit ugly, but I'm not familiar with pytest fixtures. @clemens-fricke do you have an idea of how I could make it better?
Specifically this part for example:
@pytest.fixture
def faces_quad(vertices_3d, quad_connec):
return gus.Faces(vertices_3d, quad_connec)
@pytest.fixture
def faces_quad_2d(vertices_2d, quad_connec_2d):
return gus.Faces(vertices_2d, quad_connec_2d)Is there maybe a way to include the dimensionality as a parameter? Or should we leave it like this?
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (5)
- gustaf/io/mfem.py (3 hunks)
- tests/conftest.py (5 hunks)
- tests/data/mfem_quadrilaterals_2d.mesh (1 hunks)
- tests/data/mfem_triangles_2d.mesh (1 hunks)
- tests/test_export.py (1 hunks)
Additional comments not posted (11)
tests/data/mfem_quadrilaterals_2d.mesh (1)
1-23: File Approved: MFEM 2D Quadrilateral Mesh.The file correctly defines a 2D quadrilateral mesh in the MFEM format, with appropriate elements, boundary conditions, and vertices.
tests/data/mfem_triangles_2d.mesh (1)
1-24: File Approved: MFEM 2D Triangle Mesh.The file correctly defines a 2D triangular mesh in the MFEM format, with appropriate elements, boundary conditions, and vertices.
tests/conftest.py (6)
39-50: LGTM!The
vertices_2dfixture is well-defined and provides a clear set of 2D vertices.
107-116: LGTM!The
tri_connec_2dfixture is well-defined and provides a clear 2D triangle connectivity array.
124-126: LGTM!The
faces_tri_2dfixture is well-defined and correctly creates face objects using 2D vertices and triangle connectivity.
145-153: LGTM!The
quad_connec_2dfixture is well-defined and provides a clear 2D quadrilateral connectivity array.
161-163: LGTM!The
faces_quad_2dfixture is well-defined and correctly creates face objects using 2D vertices and quadrilateral connectivity.
291-294: Improve the docstring for clarity.The current docstring does not clearly explain the function's behavior and parameters.
- """given tmpd, returns tmpf""" + """ + Given a temporary directory path `tmpd`, returns a temporary file path `tmpf`. + """Likely invalid or redundant comment.
gustaf/io/mfem.py (3)
229-232: EnsureBCis a dictionary.Before using
BC, ensure it is a dictionary to avoid potential errors.+ if not isinstance(BC, dict): + raise TypeError("BC must be a dictionary with boundary IDs as keys and lists of face IDs as values.")Likely invalid or redundant comment.
120-123: Clarify theBCparameter in the docstring.Mention that
BCis optional and explain the default behavior if not provided.- BC: dict - Boundary conditions for 3D case. - Keys are boundary IDs and values are lists of face IDs. - If none are provided, all boundary faces are assigned to 1. + BC: dict, optional + Boundary conditions for 3D case. Keys are boundary IDs and values + are lists of face IDs. If not provided, all boundary faces are + assigned to 1.Likely invalid or redundant comment.
238-239: Check for emptyfaceidslist.Ensure that
faceidsis not empty before processing to avoid potential errors.+ if not faceids: + continueLikely invalid or redundant comment.
| with open(tmpf) as tmp_read, open( | ||
| os.path.dirname(__file__) + f"/./data/{ground_truth_filename}" | ||
| ) as base_file: | ||
| print(base_file.readlines()) |
There was a problem hiding this comment.
Enhance Test Readability and Debugging.
The test function is well-structured, but consider removing or commenting out the print statement used for debugging, as it may clutter the test output.
Apply this diff to remove the print statement:
- print(base_file.readlines())Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print(base_file.readlines()) |
| @pytest.fixture | ||
| def are_stripped_lines_same(): | ||
| def _are_stripped_lines_same(a, b, ignore_order=False): | ||
| """returns True if items in a and b same, preceding and tailing | ||
| whitespaces are ignored and strings are joined""" |
There was a problem hiding this comment.
Improve the docstring for clarity.
The current docstring does not clearly explain the function's behavior and parameters.
- """returns True if items in a and b same, preceding and tailing
- whitespaces are ignored and strings are joined"""
+ """
+ Returns True if items in `a` and `b` are the same after stripping preceding
+ and trailing whitespaces. If `ignore_order` is True, checks if the words
+ in the lines match in any order.
+ """Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.fixture | |
| def are_stripped_lines_same(): | |
| def _are_stripped_lines_same(a, b, ignore_order=False): | |
| """returns True if items in a and b same, preceding and tailing | |
| whitespaces are ignored and strings are joined""" | |
| @pytest.fixture | |
| def are_stripped_lines_same(): | |
| def _are_stripped_lines_same(a, b, ignore_order=False): | |
| """ | |
| Returns True if items in `a` and `b` are the same after stripping preceding | |
| and trailing whitespaces. If `ignore_order` is True, checks if the words | |
| in the lines match in any order. | |
| """ |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- gustaf/io/mfem.py (1 hunks)
Additional comments not posted (5)
gustaf/io/mfem.py (5)
128-130: Enhance docstring to reflect new functionality.The docstring should mention the support for boundary conditions explicitly.
136-139: Clarify theBCparameter in the docstring.Mention that
BCis optional and explain the default behavior if not provided.
258-274: Check for emptyfaceidslist.Ensure that
faceidsis not empty before processing to avoid potential errors.
237-247: Ensure correct handling of 3D mesh types.Verify that the logic for handling different 3D mesh types (tetrahedron and hexahedron) is correct and that the
NotImplementedErroris raised for unsupported types.Run the following script to verify the handling of 3D mesh types:
Verification successful
3D Mesh Types Handling Verified
The handling of 3D mesh types (tetrahedron and hexahedron) in the
_export_3dfunction is correctly implemented. Unsupported types are handled with aNotImplementedError.
- The code correctly assigns geometry types for "tet" and "hexa".
- Unsupported mesh types raise a
NotImplementedError.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the handling of 3D mesh types in the _export_3d function. # Test: Search for the handling of 3D mesh types. Expect: Correct handling of tetrahedron and hexahedron types. rg --type python -A 5 $'mesh.whatami == "tet"'Length of output: 78
Script:
#!/bin/bash # Description: Verify the handling of 3D mesh types in the _export_3d function. # Corrected Test: Search for the handling of 3D mesh types. Expect: Correct handling of tetrahedron and hexahedron types. rg -A 5 'mesh.whatami == "tet"' rg -A 5 'mesh.whatami == "hexa"'Length of output: 754
148-155: Verify dimension handling logic.Ensure that the logic for handling different mesh dimensions is correct and that the
NotImplementedErroris raised appropriately for unsupported dimensions.Run the following script to verify the dimension handling logic:
Verification successful
Dimension Handling Logic Verified
The logic for handling mesh dimensions in
gustaf/io/mfem.pyis correctly implemented. The dimension is determined usingmesh.vertices.shape[1], and the code appropriately handles 2D and 3D meshes, raising aNotImplementedErrorfor unsupported dimensions. No issues were found.
gustaf/io/mfem.py: Correct handling of 2D and 3D dimensions with appropriate error handling for others.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the dimension handling logic in the export function. # Test: Search for the dimension handling logic. Expect: Correct handling of 2D and 3D dimensions. rg --type python -A 5 $'dim = mesh.vertices.shape[1]'Length of output: 85
Script:
#!/bin/bash # Description: Verify the dimension handling logic in the export function. # Test: Search for the dimension handling logic. Expect: Correct handling of 2D and 3D dimensions. rg --type py -A 5 $'dim = mesh.vertices.shape[1]'Length of output: 50
Script:
#!/bin/bash # Description: Search for all occurrences of 'dim' assignments and related logic. # Search for any assignment to 'dim' in Python files. rg --type py 'dim\s*=' -A 5Length of output: 2846
Added feature to export MFEM mesh containing tetrahedra.
Small working example:
Summary by CodeRabbit
New Features
Tests
Data