-
Notifications
You must be signed in to change notification settings - Fork 0
feat: VTK filters for mesh-doctor Part 01 Base classes and io #143
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
feat: VTK filters for mesh-doctor Part 01 Base classes and io #143
Conversation
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.
It seems good for me, thanks for having split your pr, it is easier to review.
You just may add unit case to your test for generic helper in order test more cases.
self: Self, | ||
mesh: vtkUnstructuredGrid, | ||
filterName: str, | ||
useExternalLogger: bool = False, |
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.
To be more specific, users can use an external Handler more than an external logger
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.
As this refers to the same variable that is called speHandler
in the other filters, it is a bit confusing to change it only here... I suggest to keep that same name here and dedicate a specific PR to change this name in all files if needed.
Args: | ||
filepath (str): /path/to/your/file.vtu | ||
isDataModeBinary (bool, optional): Writes the file in binary format or ascii. Defaults to True. |
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.
isDataModeBinary (bool, optional): Writes the file in binary format or ascii. Defaults to True. | |
isDataModeBinary (bool, optional): Writes the file in binary format (True) or ascii (False). Defaults to True. |
Defaults to False. | ||
""" | ||
if self.mesh: | ||
vtk_output = VtkOutput( filepath, isDataModeBinary ) |
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.
Why do you use snake case and not camel case here and after?
Args: | ||
filepath (str): /path/to/your/file.vtu | ||
isDataModeBinary (bool, optional): Writes the file in binary format or ascii. Defaults to True. |
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.
isDataModeBinary (bool, optional): Writes the file in binary format or ascii. Defaults to True. | |
isDataModeBinary (bool, optional): Writes the file in binary format (True) or ascii (False). Defaults to True. |
mesh (vtkPointSet): The grid data to write. | ||
writer_class (VtkWriterClass): The VTK writer class to use. | ||
output (str): The output file path. | ||
is_binary (bool): Whether to write the file in binary mode. |
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.
is_binary (bool): Whether to write the file in binary mode. | |
is_binary (bool): Whether to write the file in binary mode (True) or ASCII (False). |
# If it does not match, then test all the others. | ||
for reader in extension_to_reader.values(): | ||
output_mesh = reader( vtk_input_file ) | ||
filepath_path: Path = Path( filepath ) |
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.
Why not directly ask for a Path instead of str ?
io_logger.warning( f"Unknown file extension '{filepath_path.suffix}'. Trying all available readers." ) | ||
|
||
# 2. Add all other unique readers as fallbacks | ||
for reader_cls in READER_MAP.values(): |
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.
Why not use vtkXMLGenericDataObjectReader ?
|
||
|
||
def test_findUniqueCellCenterCellIds() -> None: | ||
"""Test of findUniqueCellCenterCellIds method.""" |
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.
You may test your function with two cells sharing a center but with diferents shape for example with a tetra with the following coordinate [ [ -1, -1, -1 ], [ 4, -1, -1 ], [ -1, 4, -1 ], [ -1, -1, 4 ] ] or [ [ 0, 0, 0 ], [ 2, 0, 0 ], [ 0, 0, 2 ], [ -1, -1, 4 ] ] to be sure to catch all the possibilities
MeshDoctorFilterBase serves as the foundation class for filters that process existing meshes, | ||
while MeshDoctorGenerator is for filters that generate new meshes from scratch. | ||
These base classes provide common functionality including: |
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.
These base classes provide common functionality including: | |
These base classes provide common functionalities including: |
"""Writes a .vtu file of the vtkUnstructuredGrid at the specified filepath. | ||
Args: | ||
filepath (str): /path/to/your/file.vtu |
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.
filepath (str): /path/to/your/file.vtu | |
filepath (str): The path for the output mesh file. |
if not useExternalLogger: | ||
self.logger = getLogger( filterName, True ) | ||
else: | ||
import logging |
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.
I suggest to move this import at the top of the file as it better for readability of the code. The logging
package is one of the most common and 'cheap' package, the import without actually using it will not be too bothersome.
class MeshDoctorGeneratorBase: | ||
"""Base class for mesh doctor generator filters (no input mesh required). | ||
This class provides functionality for filters that generate meshes |
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.
This class provides functionality for filters that generate meshes | |
This class provides functionalities for filters that generate meshes |
def copyMesh( self: Self, sourceMesh: vtkUnstructuredGrid ) -> vtkUnstructuredGrid: | ||
"""Helper method to create a copy of a mesh with structure and attributes. | ||
Args: | ||
sourceMesh (vtkUnstructuredGrid): Source mesh to copy from. | ||
Returns: | ||
vtkUnstructuredGrid: New mesh with copied structure and attributes. | ||
""" | ||
output_mesh: vtkUnstructuredGrid = sourceMesh.NewInstance() | ||
output_mesh.CopyStructure( sourceMesh ) | ||
output_mesh.CopyAttributes( sourceMesh ) | ||
return output_mesh |
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.
This function is generic and could be moved to geos-mesh.utils
.
self: Self, | ||
mesh: vtkUnstructuredGrid, | ||
filterName: str, | ||
useExternalLogger: bool = False, |
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.
As this refers to the same variable that is called speHandler
in the other filters, it is a bit confusing to change it only here... I suggest to keep that same name here and dedicate a specific PR to change this name in all files if needed.
"""Writes a .vtu file of the vtkUnstructuredGrid at the specified filepath. | ||
Args: | ||
filepath (str): /path/to/your/file.vtu |
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.
filepath (str): /path/to/your/file.vtu | |
filepath (str): The path for the output mesh file. |
is_binary (bool): Whether to write the file in binary mode. | ||
Returns: | ||
int: The result of the write operation. |
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.
int: The result of the write operation. | |
int: 1 if success, 0 otherwise. |
This PR aims to split the large PR #124 into multiple steps while still keeping the same logic to resolve #80
I am considering following this PR by at least three other PRs completing this work, the last one being the complete .rst documentation.
Once done, I will close PR #124
This first part involves: