Correction of erroneous copy of pre-split tags to new split meshes #34
Conversation
…l mesh to the 2 split meshes. But the indexes of fields are not correct yet for fracture mesh.
Updating sphinx and sphinx-argparse solved the issue.
…l mesh to the 2 split meshes. But the indexes of fields are not correct yet for fracture mesh.
…ps://github.com/GEOS-DEV/geosPythonPackages into origin/benedicto/bugfix/issue#31_pre_split_tags
|
@ryar9534 Already did a first review with me and said it looked fine. |
| @@ -23,7 +23,7 @@ | |||
| vtkXMLMultiBlockDataReader, ) | |||
| from vtkmodules.util.numpy_support import ( | |||
| vtk_to_numpy, ) | |||
| from vtk_utils import ( | |||
| from .vtk_utils import ( | |||
There was a problem hiding this comment.
absolute imports are the recommended import style per the PEP 8 styling guide
| @@ -1,5 +1,6 @@ | |||
| from collections import defaultdict | |||
| from dataclasses import dataclass | |||
| from sys import exit | |||
There was a problem hiding this comment.
never use exit in a function. return an error code like -1 or an exception
| cells_nodes_coordinates: dict[ int, list[ tuple[ float ] ] ] = {} | ||
| for i in range( mesh.GetNumberOfCells() ): | ||
| cell: vtkCell = mesh.GetCell( i ) | ||
| cells_nodes_coordinates[ i ] = [] |
There was a problem hiding this comment.
list() is more comprehensive for empty lists than square brackets.
| for v in range( cell.GetNumberOfPoints() ): | ||
| node_coordinates: tuple[ float ] = cell_points.GetPoint( v ) | ||
| truncated_coordinates: list[ float ] = [ float( '%.3f' % ( coord ) ) for coord in node_coordinates ] | ||
| cells_nodes_coordinates[ i ].append( tuple( truncated_coordinates ) ) |
There was a problem hiding this comment.
I don't get the idea. You get a tuple, put it in a list via float cutting and the back to tuple ? btw python float has double precision on almost all platforms. There is not need to cast them
| dict[int, int]: { new_cell_id: old_cell_id } | ||
| """ | ||
| new_cell_ids_with_old_cell_ids: dict[ int, int ] = {} | ||
| cpc_old_mesh: dict[ int, list[ tuple[ float ] ] ] = cells_points_coordinates( old_mesh ) |
There was a problem hiding this comment.
is possible to have a TypeAlias for this one. It would make it more human-friendly
| @@ -46,6 +46,19 @@ class TestCase: | |||
| result: TestResult | |||
|
|
|||
|
|
|||
| class QuadCoords: | |||
|
|
|||
| def __init__( self, p1, p2, p3, p4 ): | |||
| data.AddArray( vtk_array ) | ||
|
|
||
|
|
||
| def find_min_max_coords_rectilinear_grid( mesh: vtkUnstructuredGrid ) -> tuple[ list[ float ] ]: |
There was a problem hiding this comment.
her you should look at GetPoints.GetArray.GetRange instead of recomputing it in python
| tuple[QuadCoords]: For a rectilinear grid, returns a tuple of 6 elements. | ||
| """ | ||
| min_coords, max_coords = find_min_max_coords_rectilinear_grid( mesh ) | ||
| center: tuple[ float ] = ( ( min_coords[ 0 ] + max_coords[ 0 ] ) / 2, ( min_coords[ 1 ] + max_coords[ 1 ] ) / 2, |
There was a problem hiding this comment.
are you trying to compute the bounding box of a rectilinearGrid ? Look at boundingBox directly into VTK
| coordinates (QuadCoords): A QuadCoords containing 4 points coordinates. | ||
| """ | ||
| points_coords = mesh.GetPoints().GetData() | ||
| numpy_coordinates: numpy.array = vtk_to_numpy( points_coords ) |
There was a problem hiding this comment.
vtk_to_numpy isn't necessary if you don't use numpy afterwards
| mesh.InsertNextCell( quad.GetCellType(), quad.GetPointIds() ) | ||
|
|
||
|
|
||
| def add_quads_to_all_borders_rectilinear_grid( mesh: vtkUnstructuredGrid ): |
There was a problem hiding this comment.
I don't understand why all these tiny functions that don't have any sens alone. If the atomic task is "create a quad face per bounding box face" just do it in one function.
|
+1 for using the typing system |
| @@ -252,54 +227,154 @@ def __call__( self, index: int ) -> int: | |||
| return result | |||
|
|
|||
|
|
|||
| def __copy_fields( old_mesh: vtkUnstructuredGrid, new_mesh: vtkUnstructuredGrid, | |||
| collocated_nodes: Sequence[ int ] ) -> None: | |||
| def cells_points_coordinates( mesh: vtkUnstructuredGrid ) -> CellsPointsCoords: | |||
There was a problem hiding this comment.
I recommand a function called ```truncate_coordinates`` that gets the Point Array and truncates all points. I think you do not need to use adjacencies to only truncated coords
There was a problem hiding this comment.
I am not sure to follow.
The goal of this function is to create a mapping of cells with their truncated points coordinates so that when I use the function link_new_cells_id_with_old_cells_id, I am able to compare cell by cell the points coordinates and determine the ID mapping of cells from the old mesh to the cells of the new mesh.
To only truncate the coordinates without making the mapping with the adjacency at the same time would still imply that I will have to do it later.
Maybe renamimg the function to cells_points_truncated_coordinates would make more understandable.
|
As a general note, if you would like to disable the yapf formatter for a block of code (say for large structured datasets), then you can do the following: # yapf: disable
FOO = {
# ... some very large, complex data literal.
}
BAR = [
# ... another large data literal.
]
# yapf: enable |
| @@ -1,30 +1,14 @@ | |||
| from dataclasses import dataclass | |||
| import logging | |||
There was a problem hiding this comment.
We should be creating our own logger instance that we can set verbosity level on:
import logging
logger = logging.getLogger('mesh_doctor')
# Use the specific logger throughout the code
logger.warning('Some warning message')
# Note: somewhere in the entry point of the code, we can accept a user argument to set the level
logger.setLevel('WARNING')There was a problem hiding this comment.
I can also do that in another PR because this is done also in all 'check' files
|
@alexbenedicto - do you think that the mesh_doctor unit tests are in a good enough state to enable them in the CI? I'd be happy to help configure them for you. |
|
@cssherman From what I have seen, a lot of them would not run yet because of invalid module import. |
Sure, would you like to do this in this or a future PR? |
|
@cssherman Future PR |
|
@untereiner @cssherman do you approve to merge ? |
untereiner
deleted the
origin/benedicto/bugfix/issue#31_pre_split_tags
branch
Linked to issue #31.
When using mesh_doctor generate_fractures feature, the pre-tags used to identify fracture surfaces to split the mesh with would not be copied into the two new mesh generated (main_mesh and fracture_mesh).
To illustrate, if the tag in the pre-mesh used to identify the fracture surface was a CellData array called "CellEntityIds", the two new mesh generated would not have this "CellEntityIds" array anymore.
It appeared that not only the field used to identify the fracture surface was missing splitting, but any cell, field or point data array would not be copied too. This has been corrected and pre-mesh fields are now copied in the two new meshes.
Note :
TO USE GLOBAL IDS
The correct workflow to work with global ids is:
Doing the opposite would trigger an error message that would stop the generation of the two new meshes.