refactor: Move `PVMergeBlocksEnhanced` plugin and create the VTK filter #129

paloma-martinez · 2025-09-02T14:55:51Z

Closes #92

…iblockdataset

…handling

…TKFilter

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

geos-mesh/src/geos/mesh/processing/MergeBlockEnhanced.py

geos-pv/src/geos/pv/plugins/PVMergeBlocksEnhanced.py

…TKFilter

jafranc

IMO it may help to raise error and keep the old semantics of mergeBlocks() other than that 👍

jafranc · 2025-09-18T09:13:56Z

geos-mesh/src/geos/mesh/processing/CreateConstantAttributePerRegion.py

 )

-from geos.utils.Logger import ( getLogger, Logger, logging, CountWarningHandler )
+from geos.utils.Logger import getLogger, Logger, CountWarningHandler


Let's keep parenthesis when doing multiple import other than standard

Suggested change

from geos.utils.Logger import getLogger, Logger, CountWarningHandler

from geos.utils.Logger import ( getLogger, Logger, CountWarningHandler )

geos-mesh/src/geos/mesh/processing/MergeBlockEnhanced.py

jafranc · 2025-09-18T09:19:31Z

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

                                            vtkUnstructuredGrid )
 from vtkmodules.vtkFiltersCore import vtkAppendDataSets
 from geos.mesh.utils.arrayModifiers import fillAllPartialAttributes
+from geos.utils.Logger import getLogger, Logger


Suggested change

from geos.utils.Logger import getLogger, Logger

from geos.utils.Logger import ( getLogger, Logger )

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

jafranc · 2025-09-18T09:31:55Z

geos-posp/src/PVplugins/PVMohrCirclePlot.py

+        meshMerged: vtkUnstructuredGrid
+        if isinstance( mesh, vtkMultiBlockDataSet ):
+            _, meshMerged = mergeBlocks( mesh )
+        else:
+            meshMerged = mesh


wouldn't it be simpler if the behavior of mergeBlocks(mesh) returned mesh if not a vtkMultiBlockDataSet with warning ?

jafranc · 2025-09-18T09:33:49Z

geos-posp/src/geos_posp/filters/GeosBlockMerge.py


        # merge blocks
-        return mergeBlocks( compositeBlock )
+        _, mergedBlocks = mergeBlocks( compositeBlock )


Suggested change

_, mergedBlocks = mergeBlocks( compositeBlock )

mergedBlocks = vtkUnstructuredGrid()

_, mergedBlocks = mergeBlocks( compositeBlock )

would help typecheck (maybe)

jafranc · 2025-09-18T09:41:44Z

geos-posp/src/PVplugins/PVAttributeMapping.py

                fillPartialAttributes( serverMesh, attributeName, False )

            mergedServerMesh: vtkUnstructuredGrid
            if isinstance( serverMesh, vtkUnstructuredGrid ):


Maybe it would be easier to return the merged mesh if composite or multiblock, the unstructured grid input if unstructured and raise an error if other case occur that you could catch here

jafranc · 2025-09-18T09:45:50Z

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

+    if not inputMesh.IsA( "vtkMultiBlockDataSet" ) and not inputMesh.IsA( "vtkCompositeDataSet" ):
+        logger.error(
+            "The input mesh should be either a vtkMultiBlockDataSet or a vtkCompositeDataSet. Cannot proceed with the block merge."
+        )
+        return False, outputMesh
+
+    # Fill the partial attributes with default values to keep them during the merge.
+    if keepPartialAttributes and not fillAllPartialAttributes( inputMesh, logger ):
+        logger.error( "Failed to fill partial attributes. Cannot proceed with the block merge." )
+        return False, outputMesh
+
+    af: vtkAppendDataSets = vtkAppendDataSets()


Maybe raise a user define error there to avoid the Tuple return

I agree with @jafranc here, this function should only return a vtkUnstructuredGrid and raise errors when failure.

alexbenedicto

As suggested by @jafranc , the mergeBlocks function needs to be reworked to raise errors which will require error handling in all the scripts using it

alexbenedicto · 2025-09-26T22:18:04Z

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

+    if not inputMesh.IsA( "vtkMultiBlockDataSet" ) and not inputMesh.IsA( "vtkCompositeDataSet" ):
+        logger.error(
+            "The input mesh should be either a vtkMultiBlockDataSet or a vtkCompositeDataSet. Cannot proceed with the block merge."
+        )
+        return False, outputMesh
+
+    # Fill the partial attributes with default values to keep them during the merge.
+    if keepPartialAttributes and not fillAllPartialAttributes( inputMesh, logger ):
+        logger.error( "Failed to fill partial attributes. Cannot proceed with the block merge." )
+        return False, outputMesh
+
+    af: vtkAppendDataSets = vtkAppendDataSets()


I agree with @jafranc here, this function should only return a vtkUnstructuredGrid and raise errors when failure.

alexbenedicto · 2025-09-26T22:19:41Z

geos-mesh/src/geos/mesh/processing/MergeBlockEnhanced.py

+
+        success: bool
+        outputMesh: vtkUnstructuredGrid
+        success, outputMesh = mergeBlocks( self.inputMesh, True, self.logger )


Following the comments made on mergeBlocks function, a try/except block can be used here to evaluate the success of the mergeBlocks function

…TKFilter

paloma-martinez · 2025-09-29T12:57:30Z

@jafranc @alexbenedicto I changed the implementation of the mergeBlocks function, now it should either return the input mesh if it is already a single block, or VTK filters called should raise errors that will be caught by the MergeBlockEnhanced filter

geos-mesh/src/geos/mesh/processing/MergeBlockEnhanced.py

jafranc

I think is we want to properly propagate the error, we should raise it where it happens mergeBlock and deal with it where it makes most sense and where it can either be recovered from or end cleanly.

In most cases I think it is in the PV plugins, and if used outside of it, we should document the errors raised.

I'll push a commit with a proposal for this case

geos-mesh/src/geos/mesh/processing/MergeBlockEnhanced.py

jafranc · 2025-10-03T14:17:08Z

geos-posp/src/geos_posp/filters/GeosBlockMerge.py


        # merge blocks
-        return mergeBlocks( compositeBlock )
+        mergedBlocks = mergeBlocks( compositeBlock )


Suggested change

mergedBlocks = mergeBlocks( compositeBlock )

try:

return mergeBlocks( compositeBlock )

except TypeError:

raise TypeError

Any reason to assign and return instead of simply return ?

geos-mesh/src/geos/mesh/processing/MergeBlockEnhanced.py

jafranc · 2025-10-03T14:37:09Z

geos-pv/src/geos/pv/plugins/PVMergeBlocksEnhanced.py

+
+        if success:
+            outputMesh.ShallowCopy( filter.getOutput() )
+            outputMesh.Modified()


Suggested change

outputMesh.Modified()

try:

filter.applyFilter()

except PPTypeError:

filter.logger.error("MergeBlocks fails due to TypeError")

else:

outputMesh.ShallowCopy( filter.getOutput() )

outputMesh.Modified()

jafranc

These last 4-5 commits are proposal on how to handle errors.

As we are dealing with vtkLogger error that are rather registering callbacks than using handlers, there is quite a workaround throwing everything into a buffer to throw on match (here with vtkExecutive ) but could/should be improved.

This is open to comments and rewrite/extension/removal.

I also create an specific VTKError class that is the first of specific PVPluginError (hopefully) if go this path and chose to raise errors rather deep and catch them in RequestData() or other high level.

…https://github.com/GEOS-DEV/geosPythonPackages into pmartinez/refactor/createMergeBlockEnhancedVTKFilter

paloma-martinez added 4 commits September 2, 2025 11:04

Modify isAttributeGlobal function to take into account recursive mult…

06eca95

…iblockdataset

Add bool output to mergeBlocks function for better logging and error …

8651018

…handling

Adding a VTK filter for enhanced merge block

fbe6bf2

Move and update plugin

b9855ce

paloma-martinez self-assigned this Sep 2, 2025

paloma-martinez added type: feature DO NOT MERGE! labels Sep 2, 2025

paloma-martinez added 10 commits September 12, 2025 11:41

Merge branch 'main' into pmartinez/refactor/createMergeBlockEnhancedV…

21d8b7d

…TKFilter

Clean

54c70eb

Fix import format

2de326a

Typing

3d20b99

Adding tests and data for test for merge blocks

22828f7

Documentation

29a038b

Yapf

3d153cd

Yapf again

af3990e

Yapf

d9f289d

Fix docstring and typing

14bcb85

paloma-martinez requested review from RomainBaville, alexbenedicto and jafranc and removed request for RomainBaville September 17, 2025 15:18

paloma-martinez added flag: ready for review and removed DO NOT MERGE! labels Sep 17, 2025

RomainBaville approved these changes Sep 18, 2025

View reviewed changes

paloma-martinez added 2 commits September 18, 2025 11:14

Fix from Romain review

05a1c17

Merge branch 'main' into pmartinez/refactor/createMergeBlockEnhancedV…

df24923

…TKFilter

jafranc reviewed Sep 18, 2025

View reviewed changes

alexbenedicto reviewed Sep 26, 2025

View reviewed changes

Modification of mergeBlocks function following review's comment

f771dd6

paloma-martinez added 3 commits September 29, 2025 11:30

Merge branch 'main' into pmartinez/refactor/createMergeBlockEnhancedV…

4d9c3d0

…TKFilter

yapf

b3112f9

bad merge fix

b1b681b

paloma-martinez requested review from alexbenedicto and jafranc September 29, 2025 12:57

jafranc reviewed Oct 3, 2025

View reviewed changes

geos-mesh/src/geos/mesh/processing/MergeBlockEnhanced.py Outdated Show resolved Hide resolved

jafranc reviewed Oct 3, 2025

View reviewed changes

jafranc added 4 commits October 3, 2025 21:33

first error handling version

682f50e

adding Fails test

7da34e9

First completed attempt to capture errors from VTK

43dd2d1

last fixes

396ed11

jafranc reviewed Oct 7, 2025

View reviewed changes

jafranc and others added 8 commits October 7, 2025 15:55

ruff + yapf

e0ab6fc

yapf tests

ba9e2b1

Update python-package.yml as order might matters

f8d2b1a

clean up

b8b1481

Merge branch 'pmartinez/refactor/createMergeBlockEnhancedVTKFilter' of …

cd48656

…https://github.com/GEOS-DEV/geosPythonPackages into pmartinez/refactor/createMergeBlockEnhancedVTKFilter

CI is testing vtk9.5.1 so skip the failing test for now

b839532

mypy and yapf

262932a

wrong version dispatch

b98b49e

	from geos.utils.Logger import getLogger, Logger, CountWarningHandler
	from geos.utils.Logger import ( getLogger, Logger, CountWarningHandler )

	_, mergedBlocks = mergeBlocks( compositeBlock )
	mergedBlocks = vtkUnstructuredGrid()
	_, mergedBlocks = mergeBlocks( compositeBlock )

refactor: Move PVMergeBlocksEnhanced plugin and create the VTK filter #129

Are you sure you want to change the base?

refactor: Move PVMergeBlocksEnhanced plugin and create the VTK filter #129

Uh oh!

Conversation

paloma-martinez commented Sep 2, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jafranc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

alexbenedicto left a comment

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 commented Sep 29, 2025

Uh oh!

Uh oh!

jafranc left a comment

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!

jafranc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

refactor: Move `PVMergeBlocksEnhanced` plugin and create the VTK filter #129

refactor: Move `PVMergeBlocksEnhanced` plugin and create the VTK filter #129