refactor: refactor vtk filter without VTKPythonAlgoritmBase #181
Conversation
RomainBaville
requested review from
alexbenedicto,
jafranc and
paloma-martinez
alexbenedicto
left a comment
alexbenedicto
left a comment
There was a problem hiding this comment.
The plugins work, just the try / except implementation to rework and it should be good.
| try: | ||
| # Count the number of cells before splitting. Then we will be able to know how many new cells and points | ||
| # to allocate because each cell type is splitted in a known number of new cells and points. | ||
| nbCells: int = self.inputMesh.GetNumberOfCells() | ||
| counts: CellTypeCounts = self._getCellCounts() | ||
| nbTet: int = counts.getTypeCount( VTK_TETRA ) # will divide into 8 tets | ||
| nbPyr: int = counts.getTypeCount( VTK_PYRAMID ) # will divide into 6 pyramids and 4 tets so 10 new cells | ||
| nbHex: int = counts.getTypeCount( VTK_HEXAHEDRON ) # will divide into 8 hexes | ||
| nbTriangles: int = counts.getTypeCount( VTK_TRIANGLE ) # will divide into 4 triangles | ||
| nbQuad: int = counts.getTypeCount( VTK_QUAD ) # will divide into 4 quads | ||
| nbPolygon: int = counts.getTypeCount( VTK_POLYGON ) | ||
| nbPolyhedra: int = counts.getTypeCount( VTK_POLYHEDRON ) | ||
| assert counts.getTypeCount( VTK_WEDGE ) == 0, "Input mesh contains wedges that are not currently supported." | ||
| # Current implementation only supports meshes composed of either polygons or polyhedra | ||
| assert nbPolyhedra * nbPolygon == 0, "Input mesh is composed of both polygons and polyhedra, but it must contains only one of the two." | ||
| nbNewPoints: int = 0 | ||
| nbNewPoints = nbHex * 19 + nbTet * 6 + nbPyr * 9 if nbPolyhedra > 0 else nbTriangles * 3 + nbQuad * 5 | ||
| nbNewCells: int = nbHex * 8 + nbTet * 8 + nbPyr * 10 + nbTriangles * 4 + nbQuad * 4 | ||
|
|
||
| self.points = vtkPoints() | ||
| self.points.DeepCopy( self.inputMesh.GetPoints() ) | ||
| self.points.Resize( self.inputMesh.GetNumberOfPoints() + nbNewPoints ) | ||
|
|
||
| self.cells = vtkCellArray() | ||
| self.cells.AllocateExact( nbNewCells, 8 ) | ||
| self.originalId = vtkIdTypeArray() | ||
| self.originalId.SetName( "OriginalID" ) | ||
| self.originalId.Allocate( nbNewCells ) | ||
| self.cellTypes = [] | ||
| # Define cell type to splitting method mapping | ||
| splitMethods = { | ||
| VTK_HEXAHEDRON: self._splitHexahedron, | ||
| VTK_TETRA: self._splitTetrahedron, | ||
| VTK_PYRAMID: self._splitPyramid, | ||
| VTK_TRIANGLE: self._splitTriangle, | ||
| VTK_QUAD: self._splitQuad, | ||
| } | ||
| for c in range( nbCells ): | ||
| cell: vtkCell = self.inputMesh.GetCell( c ) | ||
| cellType: int = cell.GetCellType() | ||
| splitMethod = splitMethods.get( cellType ) | ||
| if splitMethod is not None: | ||
| splitMethod( cell, c ) | ||
| else: | ||
| raise TypeError( | ||
| f"Cell type { vtkCellTypes.GetClassNameFromTypeId( cellType ) } is not supported." ) | ||
| # add points and cells | ||
| self.outputMesh.SetPoints( self.points ) | ||
| self.outputMesh.SetCells( self.cellTypes, self.cells ) | ||
| # add attribute saving original cell ids | ||
| cellArrays: vtkCellData = self.outputMesh.GetCellData() | ||
| assert cellArrays is not None, "Cell data is undefined." | ||
| cellArrays.AddArray( self.originalId ) | ||
| # transfer all cell arrays | ||
| self._transferCellArrays( self.outputMesh ) | ||
| self.logger.info( f"The filter { self.logger.name } succeeded." ) | ||
| except Exception as e: | ||
| self.logger.error( f"The filter {self.logger.name } failed.\n{ e }" ) |
There was a problem hiding this comment.
This try / except implementation is weird.
Many things here do not require to be part of it. It can be done in multiple try / except for specific blocks where we would except errors, like the TypeError when parsing all cell types. It will also help to have a more understandable error output.
There was a problem hiding this comment.
I dont understand how to do multiple try/except block. Raising error after each step of the workfllow schould work as well and the error messages raise schould be enough to understand issues.
| try: | ||
| # compute cell type counts | ||
| self._counts.reset() | ||
| self._counts.setTypeCount( VTK_VERTEX, self.inputMesh.GetNumberOfPoints() ) | ||
| for i in range( self.inputMesh.GetNumberOfCells() ): | ||
| cell: vtkCell = self.inputMesh.GetCell( i ) | ||
| self._counts.addType( cell.GetCellType() ) | ||
|
|
||
| # create output table | ||
| # first reset output table | ||
| self.outTable.RemoveAllRows() | ||
| self.outTable.RemoveAllColumns() | ||
| self.outTable.SetNumberOfRows( 1 ) | ||
|
|
||
| # create columns per types | ||
| for cellType in getAllCellTypes(): | ||
| array: vtkIntArray = vtkIntArray() | ||
| array.SetName( vtkCellTypes.GetClassNameFromTypeId( cellType ) ) | ||
| array.SetNumberOfComponents( 1 ) | ||
| array.SetNumberOfValues( 1 ) | ||
| array.SetValue( 0, self._counts.getTypeCount( cellType ) ) | ||
| self.outTable.AddColumn( array ) | ||
| self.logger.info( f"The filter { self.logger.name } succeeded." ) | ||
| except AssertionError as e: | ||
| self.logger.error( f"The filter { self.logger.name } failed.\n{ e }" ) |
There was a problem hiding this comment.
Same remark here regarding the try /except implementation
| try: | ||
| self._outputMesh.ShallowCopy( self.inputMesh ) | ||
| # Compute cell type counts | ||
| self._computeCellTypeCounts() | ||
|
|
||
| Args: | ||
| request (vtkInformation): Request | ||
| inInfoVec (list[vtkInformationVector]): Input objects | ||
| outInfoVec (vtkInformationVector): Output objects | ||
| # Compute metrics and associated attributes | ||
| self._evaluateMeshQualityAll() | ||
|
|
||
| Returns: | ||
| int: 1 if calculation successfully ended, 0 otherwise. | ||
| """ | ||
| inData: vtkUnstructuredGrid = self.GetInputData( inInfoVec, 0, 0 ) | ||
| self._outputMesh = self.GetOutputData( outInfoVec, 0 ) | ||
| assert inData is not None, "Input mesh is undefined." | ||
| assert self._outputMesh is not None, "Output pipeline is undefined." | ||
| self._outputMesh.ShallowCopy( inData ) | ||
| # Compute stats summary | ||
| self._updateStatsSummary() | ||
|
|
||
| # Compute cell type counts | ||
| self._computeCellTypeCounts() | ||
| # Create field data | ||
| self._createFieldDataStatsSummary() | ||
|
|
||
| # Compute metrics and associated attributes | ||
| self._evaluateMeshQualityAll() | ||
| self._outputMesh.Modified() | ||
|
|
||
| # Compute stats summary | ||
| self._updateStatsSummary() | ||
| self.logger.info( f"The filter { self.logger.name } succeeded." ) | ||
| except Exception as e: | ||
| self.logger.error( f"The filter { self.logger.name } failed.\n{ e }" ) |
There was a problem hiding this comment.
Same remark for try / except implementation.
Here, it is even more important to know what needs to be handled. This filter was already very slow before adding the try block and it could ruin the use of PVPlugin.
There was a problem hiding this comment.
How the try/except strategy can make the computation slower ?
There was a problem hiding this comment.
OK I looked it up and it is not as bad as I thought, I apologize for that.
If we do not raise Exceptions, the try/except block is efficient.
The cost comes from raising the exception when it occurs.
So if an event is expected to happen frequently (e.g., iterating through a loop and expecting a KeyError 50% of the time), the cost of raising and catching the exception will still dominate and be slower than an explicit check (if key in dict:) so some operations with the loop can be slow and we should be careful with it.
Another point is that with python >= 3.11, the overhead of the try block when no exception is raised is now virtually eliminated. This means that using a try/except block for code that rarely fails is now as fast as or faster than using an equivalent if/else check.
So my feedback should only aim at the logger itself with an appropriate error message to quickly identify the issue.
This pr aims to refactor the few vtk filter of geos-processing without using the class VTKPythonAlgorithmBase: