Skip to content

Conversation

@RomainBaville
Copy link
Contributor

@RomainBaville RomainBaville commented Sep 26, 2025

In the context of the code refactoring, this pr aims to:

  • Move the filter in the correct folder and update import/doc
  • Stop using VTKPythonAlgorithmBase as mother class of the filter
  • Implement a child class of vtkExtractBlock to extract geos domain directly
  • Implement a test file for the filter.

Note: A future pr will clean all the test mesh

@RomainBaville RomainBaville added the test-geos-integration Triggers the testing of geosPythonPackages import and integration in GEOS CI label Oct 22, 2025
.. Note::
The volume domain is automatically extracted, by defaults the fault and well domain are empty multiBlockDataSet.
CellElementRegion is automatically extracted, by defaults SurfaceElementRegion and SurfaceElementRegion are empty multiBlockDataSet.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CellElementRegion is automatically extracted, by defaults SurfaceElementRegion and SurfaceElementRegion are empty multiBlockDataSet.
CellElementRegion is automatically extracted, by defaults SurfaceElementRegion and WellElementRegion are empty multiBlockDataSet.

__doc__ = """
GeosBlockExtractor is a vtk filter that allows to extract from an output Geos multiBlockDataSet
all the blocks in a multiBlockDataSet of a Geos domain with its index.
GeosBlockExtractor is a vtk filter that allows to extract blocks from the ElementRegions from a GEOS output multiBlockDataset mesh.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
GeosBlockExtractor is a vtk filter that allows to extract blocks from the ElementRegions from a GEOS output multiBlockDataset mesh.
GeosBlockExtractor is a vtk filter that allows to extract the ElementRegions blocks from a GEOS output multiBlockDataset mesh.

Comment on lines 18 to 21
The three ElementRegions are:
0: CellElementRegion,
1: SurfaceElementRegion,
2: WellElementRegion,
Copy link
Collaborator

@paloma-martinez paloma-martinez Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a mix of the previous and the current doc, something like this ?

Suggested change
The three ElementRegions are:
0: CellElementRegion,
1: SurfaceElementRegion,
2: WellElementRegion,
The three ElementRegions are:
0: Volumes ( labeled `CellElementRegion` in the mesh )
1: Faults ( labeled `SurfaceElementRegion` )
2: Wells ( labeled `WellElementRegion` )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An enum for that would make it clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use the one already existing

Copy link
Contributor

@jafranc jafranc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an Enum and the outer-most property
Checks from the setters could be a plus. ➕ if not too heavy

Comment on lines 18 to 21
The three ElementRegions are:
0: CellElementRegion,
1: SurfaceElementRegion,
2: WellElementRegion,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An enum for that would make it clearer

"""Extract ElementRegions block from a GEOS output multiBlockDataset mesh."""
super().__init__()

self.geosElementRegionsName: dict[ int, str ] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This I think is an enum and eventhough should be class var and not instance var :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is indeed an enum and it is already defined in GeosOutputsConstants. I don't need to rewrite it.

elif elementRegionId == 2:
self.well = multiBlockDataSet

extractedElementRegions: ExtractedElementRegionsMesh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be accessible outside of the class ? If not then decorate it with @property

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to be accessible outside of the class to get the result of the extraction

"The logger already has an handler, to use yours set the argument 'speHandler' to True during the filter initialization."
)

def applyFilter( self: Self ) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If returned value is not used then maybe drop it, else maybe raise an error with diagnostic

_surface: vtkMultiBlockDataSet = vtkMultiBlockDataSet()
_well: vtkMultiBlockDataSet = vtkMultiBlockDataSet()

@property
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the setters actually check that the elements from the multiblock passed in arg are of the right type ? something following this logic


cell_type = cell.GetCellType()

# Use vtkCellTypes to get dimension
dimension = vtk.vtkCellTypes.GetDimension(cell_type)

if dimension == 3:
    print("This is a volume cell.")
elif dimension == 2:
    print("This is a surface cell.")
elif dimension == 1:
    print("This is a line cell.")
else:
    print("Unknown or unsupported cell typ

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idear. I have implemented a new function to get the dimension of the cells of the mesh and I test this dimension in the setters. It also test taht the mesh is a vtm

"""
return self.geosElementRegionsName[ elementRegionId ]

def AddGeosElementRegionsBlockIndex( self, elementRegionId: int ) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def AddGeosElementRegionsBlockIndex( self, elementRegionId: int ) -> None:
def AddGeosElementRegionsBlockIndex( self: Self, elementRegionId: int ) -> None:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flag: ready for review test-geos-integration Triggers the testing of geosPythonPackages import and integration in GEOS CI type: refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants