-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: Move PVGeomechanicsAnalysis to geos-pv and GeomechanicsCalculator to geos-processing #140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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[ :, :]
speHandler=True ) | ||
if not filter.logger.hasHandlers(): | ||
filter.setLoggerHandler( VTKHandler() ) | ||
filter.setGrainBulkModulus( self.getGrainBulkModulus() ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal of this pr was to refactor GeomechanicsCalculator only. The refactor of the 4 GeomechanicsWorkflow will be in another pr.
# 2. compute Geomechanical outputs in volume mesh | ||
self.computeAdditionalOutputsVolume() | ||
a = self.computeAdditionalOutputsVolume() | ||
print( a ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgotten print ?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
self.output: Union[ vtkPointSet, vtkUnstructuredGrid ] | ||
if mesh.IsA( "vtkUnstructuredGrid" ): | ||
self.output = vtkUnstructuredGrid() | ||
elif mesh.IsA( "vtkPointSet" ): | ||
self.output = vtkPointSet() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
self.logger.error( "The filter failed." ) | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
self.bulkModulus: npt.NDArray[ np.float64 ] | ||
self.shearModulus: npt.NDArray[ np.float64 ] | ||
self.youngModulus: npt.NDArray[ np.float64 ] | ||
self.poissonRatio: npt.NDArray[ np.float64 ] | ||
if self.computeYoungPoisson: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.