refactor: Move PVGeomechanicsAnalysis to geos-pv and GeomechanicsCalculator to geos-processing#140
Conversation
|
Looking at #139 , I do not understand the motivation in this PR to create a new package called "geos-processing". |
paloma-martinez
left a comment
There was a problem hiding this comment.
The paths changed in the 4 PVGeomechanicsWorklow* have to be reverted, they don't belong to this PR and the code cannot work in this state.
Apart from that, it looks good to me, I made a few suggestions
geos-processing/src/geos/processing/post_processing/GeomechanicsCalculator.py
Outdated
Show resolved
Hide resolved
geos-posp/src/PVplugins/PVGeomechanicsWorkflowVolumeSurfaceWell.py
Outdated
Show resolved
Hide resolved
geos-processing/src/geos/processing/post_processing/GeomechanicsCalculator.py
Outdated
Show resolved
Hide resolved
geos-processing/src/geos/processing/post_processing/GeomechanicsCalculator.py
Show resolved
Hide resolved
Co-authored-by: paloma-martinez <104762252+paloma-martinez@users.noreply.github.com>
…latorfilterandplugin Update to the last version of the main
jafranc
left a comment
There was a problem hiding this comment.
Good to go ! I made a suggestion on using an embedded dataclass and limited access as you are basically setting and fetching data.
Please check there (and more largely on all deltaStress)
geomechanicsCalculatorFunctions.py:L510 deltaStress[ : , :3] shouldn't be deltaStress[ :, :]
There was a problem hiding this comment.
That would be nice if we had as coding style rule to use @property and @<name>.setter to control access properly
There was a problem hiding this comment.
The goal of this pr was to refactor GeomechanicsCalculator only. The refactor of the 4 GeomechanicsWorkflow will be in another pr.
| filter.SetLogger( self.m_logger ) | ||
| filter.Update() | ||
| self.m_volumeMesh.ShallowCopy( filter.GetOutputDataObject( 0 ) ) | ||
| filter = GeomechanicsCalculator( self.m_volumeMesh, |
There was a problem hiding this comment.
IIUC PVGeo..VolumeSurface is a full superset of PVGeo..Volume, please use inheritance to ease up maintainability and kinda bijection from filter to plugins
| filter.SetLogger( self.m_logger ) | ||
| filter.Update() | ||
| self.m_volumeMesh.ShallowCopy( filter.GetOutputDataObject( 0 ) ) | ||
| filter = GeomechanicsCalculator( self.m_volumeMesh, |
There was a problem hiding this comment.
Same remarks as in PVGeo..VolumeSurface. Next PR.
| if filter.applyFilter(): | ||
| outputMesh.ShallowCopy( filter.getOutput() ) | ||
| outputMesh.Modified() | ||
|
|
||
| return 1 |
There was a problem hiding this comment.
| if filter.applyFilter(): | |
| outputMesh.ShallowCopy( filter.getOutput() ) | |
| outputMesh.Modified() | |
| return 1 | |
| if filter.applyFilter(): | |
| outputMesh.ShallowCopy( filter.getOutput() ) | |
| outputMesh.Modified() | |
| return 1 | |
| else: | |
| return 0 |
There was a problem hiding this comment.
For me, if the filter return 0 does not means that the plugin failed. I have used this condition to update the outputMesh only if the filter successfully ended. That why I didn't return 0 but 1 and the plugin return the input mesh.
There was a problem hiding this comment.
| self.output: Union[ vtkPointSet, vtkUnstructuredGrid ] | |
| if mesh.IsA( "vtkUnstructuredGrid" ): | |
| self.output = vtkUnstructuredGrid() | |
| elif mesh.IsA( "vtkPointSet" ): | |
| self.output = vtkPointSet() | |
| self.output: Union[ vtkPointSet, vtkUnstructuredGrid ] = mesh.NewInstance() |
should work the same
There was a problem hiding this comment.
| self.logger.error( "The filter failed." ) | |
| return False | |
| return False |
maybe already quite some failure messages send by the callee and it is not repeated in the other 2 conditions
There was a problem hiding this comment.
as it is basically a Compute class acting on a DataClass, I would suggest rewritting it using @dataclass and @property decorators to make it easier to get at first sight and improve access control
There was a problem hiding this comment.
I have made somthing tel me if that what you thought about
…latorfilterandplugin Update to the last verion of the main
…latorfilterandplugin Update to the last version of the main
geos-processing/src/geos/processing/post_processing/GeomechanicsCalculator.py
Outdated
Show resolved
Hide resolved
Co-authored-by: paloma-martinez <104762252+paloma-martinez@users.noreply.github.com>
…latorfilterandplugin Update to the last version of the main
There was a problem hiding this comment.
With respect to PR #142 and future clean up of certain output messages in PVGeomechanicsCalculator
This pr close #139 in the context of the code refactoring.
In addition of the issue, some choses have been made:
- The filter is move to the new folder geos-processing
- The name of the paraview plugin have been updated from PVGeomechanicsAnalysis to PVGeomechanicsCalculator to be the same as the filter.
- The four workflow plugins using the filter have been updated but their refactor need to be donne in another pr.
- The filter is not tested but works on complex case. A set of meshes to test all the filter need to be donne in another pr.