feat: VTK filters for mesh-doctor #124
Conversation
…_filters_for_mesh_doctor
…_filters_for_mesh_doctor
…_filters_for_mesh_doctor
paloma-martinez
left a comment
paloma-martinez
left a comment
There was a problem hiding this comment.
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 theCollocatedNodes::setPaintWrongSupportElementsfunction, 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.
| """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.
| filepath (str): /path/to/your/file.vtu | |
| filepath (str): The output file path. |
| 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 |
There was a problem hiding this comment.
__action can be removed here
|
See #145 where 2 new PRs will finish this work |
Closes #80