Skip to content
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

fix: copy fields mapping method for generate_fractures taking too long #49

Conversation

alexbenedicto
Copy link
Contributor

Closes #48

The first implementation was too slow and did not need to create a new mapping to identify the correct cells and points when copying fields of PointData/CellData from the input mesh to the outputs.

Results obtained with a mesh of 16800 cells and 15066 points:

With the first implementation
image

After the fix
image

Copy link

@bd713 bd713 left a comment

Choose a reason for hiding this comment

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

Looks good. Would just make minor edits for code readability and to reduce duplicate code.

@@ -14,6 +14,8 @@

__FRACTURES_OUTPUT_DIR = "fractures_output_dir"
__FRACTURES_DATA_MODE = "fractures_data_mode"
__FRACTURES_DATA_MODE_VALUES = "binary", "ascii"
Copy link

Choose a reason for hiding this comment

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

I'm new to this repo and don't know the conventions at play.
Do the leading underscores have a special meaning?
If not, I would simply follow the PEP8 recommendation, which is to write constants in capital letters and drop the leading underscores.

@@ -14,6 +14,8 @@

__FRACTURES_OUTPUT_DIR = "fractures_output_dir"
__FRACTURES_DATA_MODE = "fractures_data_mode"
__FRACTURES_DATA_MODE_VALUES = "binary", "ascii"
__FRACTURES_DATA_MODE_DEFAULT = __FRACTURES_DATA_MODE_VALUES[ 0 ]
Copy link

Choose a reason for hiding this comment

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

is this really useful?
Down the road, VtkOutput will take a boolean is_data_mode_binary, so it seems that we have no other choice but to use binary. That default cannot be changed in practice.

@@ -84,7 +88,7 @@ def convert( parsed_options ) -> Options:
]
fracture_names: list[ str ] = [ "fracture_" + frac.replace( ",", "_" ) + ".vtu" for frac in per_fracture ]
fractures_output_dir: str = parsed_options[ __FRACTURES_OUTPUT_DIR ]
fractures_data_mode: str = parsed_options[ __FRACTURES_DATA_MODE ]
fractures_data_mode: str = parsed_options[ __FRACTURES_DATA_MODE ] == __FRACTURES_DATA_MODE_DEFAULT
Copy link

Choose a reason for hiding this comment

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

fractures_data_mode seems to be a boolean and not a string.
If that's the case, and based on the VtkOutput class, we could even rename it is_fractures_data_mode_binary for clarity, and even hardcode =='binary' without using a default variable.

old_ncols: int = 1 if len( old_points_array.shape ) == 1 else old_points_array.shape[ 1 ]
# Reshape old_points_array if it is 1-dimensional
if len( old_points_array.shape ) == 1:
old_points_array = old_points_array.reshape( ( old_nrows, 1 ) )
Copy link

Choose a reason for hiding this comment

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

or maybe something shorter like

old_nrows, old_ncols = old_points_array.shape if len(old_points_array.shape) > 1 else (old_points_array.shape[0], 1)
old_points_array = old_points_array.reshape(-1, old_ncols)

new_points_array = empty( ( new_number_points, old_ncols ) )
new_points_array[ :old_nrows, : ] = old_points_array
for new_and_old_id in added_points_with_old_id:
new_points_array[ new_and_old_id[ 0 ], : ] = old_points_array[ new_and_old_id[ 1 ], : ]
Copy link

Choose a reason for hiding this comment

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

for readability, maybe could try:

for new_id, old_id in added_points_with_old_id:
    new_points_array[new_id, :] = old_points_array[old_id, :]

# Copying the cell data. The interesting cells are the ones stored in face_cell_id.
new_number_cells: int = fracture_mesh.GetNumberOfCells()
input_cell_data = old_mesh.GetCellData()
for i in range( input_cell_data.GetNumberOfArrays() ):
Copy link

Choose a reason for hiding this comment

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

At this point, I would rather try to define a generic helper function for copying data.
Something like this (untested, notably if len(indices) could be used to set the number of tuples):

    def copy_data(input_data, output_data, indices):
        for i in range(input_data.GetNumberOfArrays()):
            old_array = vtk_to_numpy(input_data.GetArray(i))
            old_nrows, old_ncols = old_array.shape if len(old_array.shape) > 1 else (old_array.shape[0], 1)
            old_array = old_array.reshape(-1, old_ncols)
            name = input_data.GetArrayName(i)
            logging.info(f"Copying data \"{name}\".")
            new_array = old_array[indices, :]
            vtk_array = numpy_to_vtk(new_array.flatten() if old_ncols > 1 else new_array)
            vtk_array.SetNumberOfComponents(old_ncols)
            vtk_array.SetNumberOfTuples(len(indices))
            vtk_array.SetName(name)
            output_data.AddArray(vtk_array)

and call it twice:

copy_data(old_mesh.GetCellData(), fracture_mesh.GetCellData(), face_cell_id)
copy_data(old_mesh.GetPointData(), fracture_mesh.GetPointData(), list(node_3d_to_node_2d.keys()))

If we were willing to pay the overhead of vtk_array -> numpy -> vtk_array, that copy_data could probably be used in __copy_fields_splitted_mesh to replace the repeated block:

   for i in range( input_cell_data.GetNumberOfArrays() ):
        input_array: vtkDataArray = input_cell_data.GetArray( i )
        logging.info( f"Copying cell field \"{input_array.GetName()}\"." )
        tmp = input_array.NewInstance()
        tmp.DeepCopy( input_array )
        splitted_mesh.GetCellData().AddArray( input_array )

@alexbenedicto alexbenedicto merged commit 1e91901 into main Dec 12, 2024
31 checks passed
@alexbenedicto alexbenedicto deleted the origin/bugfix/benedicto/copy_fields_mapping_generate_fractures branch December 12, 2024 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy of fields taking too long when splitting meshes using generate_fractures
2 participants