feat: VTK filters for mesh-doctor #124

alexbenedicto · 2025-08-13T00:33:27Z

Closes #80

…k of parallelism

…_filters_for_mesh_doctor

RomainBaville

The code is good and essay to read but some typing and docstrings are missing.

More over, I don't understand some points :

why the check check_fractures is not also check in the function AllChecks.
why the doc is duplicaded (a file.rst and a doc part in the filter files)
why you use the VTKPythonAlgorithmBase for the filter while the mesh exists already, and with the VTKPythonWrapping we use, the vtk objects are pointers. Meshes can be updated without the VTKPythonAlgorithmBase pipline.
why in the filter you use member function to set some variable (mesh, tolerance ...) while they can set as arguments.

At least, the function get_vtk_constant_str in genericHelpers needs to be updated. For now it can return a false name because severals VTK_CONSTANT refers to the same number.

geos-mesh/src/geos/mesh/doctor/actions/all_checks.py

geos-mesh/src/geos/mesh/doctor/actions/check_fractures.py

geos-mesh/src/geos/mesh/doctor/actions/all_checks.py

geos-mesh/src/geos/mesh/doctor/filters/CollocatedNodes.py

geos-mesh/src/geos/mesh/utils/genericHelpers.py

geos-mesh/src/geos/mesh/doctor/filters/Checks.py

geos-mesh/src/geos/mesh/doctor/actions/supported_elements.py

RomainBaville

It seems good for me, thanks for your answer.
I have just a comment about the logger, for the moment it works but a specific handle has to be used for paraview. With your method, it will not easy but it can be done in a future pr.

geos-mesh/src/geos/mesh/doctor/actions/element_volumes.py

RomainBaville · 2025-08-22T07:10:14Z

geos-mesh/src/geos/mesh/doctor/actions/fix_elements_orderings.py

@@ -1,8 +1,9 @@
 from dataclasses import dataclass
 from typing import Dict, FrozenSet, List, Set


Why import "List", "Dict", "FrozenSet" and "Set" from typing while "list", "dict", "frozenset" and "set" already are in python?

RomainBaville · 2025-08-22T07:18:06Z

geos-mesh/src/geos/mesh/doctor/actions/generate_cube.py

+                                                  listValues=listValues,
+                                                  attributeName=field_info.name,
+                                                  onPoints=onPoints )
+        if not success:


createConstantAttributeDataSet already log an error if the attribute is not created.

RomainBaville · 2025-08-22T07:22:55Z

geos-mesh/src/geos/mesh/doctor/actions/generate_cube.py

+        success = createConstantAttributeDataSet( dataSet=mesh,
+                                                  listValues=listValues,
+                                                  attributeName=field_info.name,
+                                                  onPoints=onPoints )


You use another logger as the internal one used by this function, you should give your logger to the function (logger=setup_logger) to be sure to keep the same dysplay and names.

RomainBaville · 2025-08-22T07:50:40Z

geos-mesh/src/geos/mesh/io/vtkIO.py


+    io_logger.info( "Reader did not match the file format." )


Why use .info and not .warning or .error? The function didn't succed to read the file.

RomainBaville · 2025-08-22T07:59:35Z

geos-mesh/src/geos/mesh/io/vtkIO.py

-    writer.SetInputData( mesh )
-    writer.SetDataModeToBinary() if toBinary else writer.SetDataModeToAscii()
-    return writer.Write()
+    if not output_mesh:


Why do you test again output_mesh? You could raise the ValueError after the for.

paloma-martinez

The general code implementation looks good to me, I have some remarks regarding the documentation and the formatting.

for the .rst files of filters, the documentation is very exhaustive (which is good ! ), but I'm wondering what's the reason for not using automodule or literalinclude functionality, at least for some parts ?
It seems there are also some cases when the code documentation is less detailed than the .rst file... For example in the CollocatedNodes::setPaintWrongSupportElements function, you don't indicate that the default value for the filter would be 0 while it is in the .rst doc.
The docstring documentation of functions is incomplete/inexistant in many files, there are the name and type of variables, but the description is often missing.
You changed the docstring format from Google docstring to reST in some files (geos-mesh/src/geos/mesh/doctor/actions/non_conformal.py), and did the opposit in some others (geos-mesh/src/geos/mesh/doctor/actions/supported_elements.py)
I'm curious about the use of m_ convention for the class attributes, it is pretty unusual in python.

docs/geos_mesh_docs/doctor.rst

docs/geos_mesh_docs/filters/GenerateFractures.rst

paloma-martinez · 2025-08-26T07:29:49Z

geos-mesh/src/geos/mesh/doctor/actions/supported_elements.py

+            """
+            Builds the face to face connectivities (through edges) for prism graphs.
+            :param n: The number of nodes of the basis (i.e. the pentagonal prims gets n = 5)
+            :param name: A human-readable name for logging purpose.
+            :return: A graph instance.


Why changing the doc formatting ?

geos-mesh/src/geos/mesh/doctor/filters/Checks.py

paloma-martinez · 2025-08-26T09:20:49Z

geos-mesh/src/geos/mesh/doctor/filters/MeshDoctorBase.py

+        """Writes a .vtu file of the vtkUnstructuredGrid at the specified filepath.
+
+        Args:
+            filepath (str): /path/to/your/file.vtu


Suggested change

filepath (str): /path/to/your/file.vtu

filepath (str): The output file path.

geos-mesh/src/geos/mesh/doctor/filters/NonConformal.py

paloma-martinez · 2025-08-26T11:53:10Z

geos-mesh/tests/test_supported_elements.py

 from typing import Tuple
 from vtkmodules.vtkCommonCore import vtkIdList, vtkPoints
-from vtkmodules.vtkCommonDataModel import vtkUnstructuredGrid, VTK_POLYHEDRON
+from vtkmodules.vtkCommonDataModel import ( vtkUnstructuredGrid, vtkQuad, vtkTetra, vtkHexahedron, vtkPolyhedron,
+                                            vtkCellArray, VTK_POLYHEDRON, VTK_QUAD, VTK_TETRA, VTK_HEXAHEDRON )
 # from geos.mesh.doctor.actions.supported_elements import Options, action, __action


__action can be removed here

docs/geos_mesh_docs/filters/GenerateRectilinearGrid.rst

alexbenedicto added 25 commits May 9, 2025 14:54

Add GenerateRectilinearGrid

ee78216

Add ElementVolumes filter

cc8e2a5

Add GenerateFractures filter

b88bff7

Add CollocatedNodes filter

d32ad33

Add SupportElements filter but without polyhedron checking due to lac…

bac1a83

…k of parallelism

To revert

f180f8f

Merge remote-tracking branch 'origin/main' into benedicto/feature/vtk…

f6bc7f2

…_filters_for_mesh_doctor

Keep supported_elements like before merging

f14cc1b

Update already implemented filters to work after merge

2f367bb

Temporary commit

0b44584

Merge remote-tracking branch 'origin/main' into benedicto/feature/vtk…

a9c4f7a

…_filters_for_mesh_doctor

Add base class for all mesh doctor VTK filters

f68b38d

Update existing filters with new base class

9727b21

Fix init for GenerateRectilinearGrid

9516825

Add SelfIntersectingElements

b9d9a78

Change __action to mesh_action function for better clarity

4838fce

Change base class names + add MeshDoctorChecks base

e7a4134

Remove MeshDoctorChecks class

41ce1a1

Fix global variable assignation for MESH in supported_elements

353abc5

Add AllChecks and MainChecks filters

0009e51

yapf on mesh-doctor

a67d3cb

Update __doc__ for each filter

686ff68

Update .rst documentations

8a07761

Merge remote-tracking branch 'origin/main' into benedicto/feature/vtk…

ed9843c

…_filters_for_mesh_doctor

Improve displayed results for supported_elements

d81951e

alexbenedicto requested review from paloma-martinez and RomainBaville August 13, 2025 00:33

alexbenedicto self-assigned this Aug 13, 2025

alexbenedicto added type: feature type: documentation labels Aug 13, 2025

yapf

dfe85b2

alexbenedicto changed the title ~~VTK filters for mesh doctor~~ feat: VTK filters for mesh-doctor Aug 13, 2025

alexbenedicto marked this pull request as ready for review August 13, 2025 01:10

RomainBaville reviewed Aug 13, 2025

View reviewed changes

alexbenedicto added 6 commits August 13, 2025 15:11

Update vtkIO to be more robust, reduce duplication

ce1ca67

Update mesh_action to only be given vtkUnstructuredGrid

bfe2517

Review implementations + yapf

f99e42f

Fix invalid variable names

71496c6

yapf

40abad6

Fix docs

ab17743

alexbenedicto requested a review from RomainBaville August 14, 2025 16:32

RomainBaville approved these changes Aug 22, 2025

View reviewed changes

paloma-martinez reviewed Aug 26, 2025

View reviewed changes

		@@ -1,8 +1,9 @@
		from dataclasses import dataclass
		from typing import Dict, FrozenSet, List, Set

	filepath (str): /path/to/your/file.vtu
	filepath (str): The output file path.

feat: VTK filters for mesh-doctor #124

Are you sure you want to change the base?

feat: VTK filters for mesh-doctor #124

Uh oh!

Conversation

alexbenedicto commented Aug 13, 2025

Uh oh!

RomainBaville left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RomainBaville left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paloma-martinez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!