Skip to content

Specify element data via element type#600

Merged
isteinbrecher merged 2 commits intobeamme-py:mainfrom
isteinbrecher:block_element_data
Apr 13, 2026
Merged

Specify element data via element type#600
isteinbrecher merged 2 commits intobeamme-py:mainfrom
isteinbrecher:block_element_data

Conversation

@isteinbrecher
Copy link
Copy Markdown
Collaborator

The first step towards #594 is that we gather similar elements in element blocks.

An element for 4C is basically defined by its connectivity and element data. Until now, we store the element data in each element instance. A better approach would be to store the element data in the element type. This would allow us to consider all elements of the same type (and material) to be of the same element block.

This PR proposes to switch the element data into the element type. This is actually more or less straightforward, as we already did something like that for the beam elements, now we also do this for NURBS and solid elements.

@isteinbrecher
Copy link
Copy Markdown
Collaborator Author

@davidrudlstorfer I am in no hurry merging this, but I would be curious about your opinion.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors element data storage for solid and NURBS elements to match the pattern already used for beam elements. Instead of storing element data in each element instance, the data is now stored in the element type itself via a class-level four_c_element_data attribute. This architectural change is the first step towards organizing elements into element blocks (issue #594), where elements of the same type and material can be grouped together.

Changes:

  • Introduced get_four_c_solid() function to create NURBS element types with element data stored at the class level
  • Updated NURBS mesh creation functions to accept element types instead of determining types internally
  • Modified element dumping and importing logic to work with class-level element data
  • Refactored input file mappings to use node count instead of element class for type string lookup

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/beamme/four_c/element_solid.py New file implementing get_four_c_solid() to create NURBS element types with class-level element data
src/beamme/mesh_creation_functions/nurbs_generic.py Updated to accept element types as parameters and removed internal type selection logic
src/beamme/four_c/input_file_dump_item.py Modified to access element data from class-level four_c_element_data attribute
src/beamme/four_c/input_file_mappings.py Refactored mappings to use node count instead of element class, reorganized beam type mappings
src/beamme/four_c/model_importer.py Added create_solid_element_type() to create element types with class-level data during import
src/beamme/four_c/solid_shell_thickness_direction.py Updated to use getattr() for safe access to class-level element data
src/beamme/four_c/element_beam.py Updated to use renamed beam_type_to_four_c_type mapping
src/beamme/four_c/element_rigid_sphere.py Corrected docstring from "volume elements" to "solid elements"
tests/integration/test_integration_mesh_creation_functions_nurbs.py Updated tests to create element types via get_four_c_solid() before passing to mesh creation functions
tests/integration/test_integration_four_c.py Updated test to create element type before adding NURBS to mesh
tests/beamme/mesh_creation_functions/test_beamme_mesh_creation_functions_nurbs_generic.py Updated test to create element type before adding NURBS to mesh

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/beamme/four_c/model_importer.py Outdated
Comment thread src/beamme/mesh_creation_functions/nurbs_generic.py Outdated
Comment thread src/beamme/mesh_creation_functions/nurbs_generic.py Outdated
Comment thread src/beamme/four_c/input_file_dump_item.py Outdated
Copy link
Copy Markdown
Collaborator

@davidrudlstorfer davidrudlstorfer left a comment

Choose a reason for hiding this comment

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

First of all I am super sorry for the long time to review this. Due to the FE exam I was busy the last weeks.

The overall PR looks really good and the code simplifies in a lot of places. More or less I only have a few questions/unclarity regarding functionality.

Comment thread src/beamme/four_c/element_rigid_sphere.py Outdated
Comment thread src/beamme/four_c/element_solid.py Outdated
Comment thread src/beamme/four_c/input_file_dump_item.py Outdated
Comment thread src/beamme/four_c/model_importer.py Outdated
Comment thread src/beamme/four_c/model_importer.py Outdated
Comment thread src/beamme/mesh_creation_functions/nurbs_generic.py Outdated
Comment thread src/beamme/mesh_creation_functions/nurbs_generic.py Outdated
@isteinbrecher
Copy link
Copy Markdown
Collaborator Author

@davidrudlstorfer thanks for the review, I hope I will be able to continue the work on this in the next weeks.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (2)

src/beamme/four_c/element_rigid_sphere.py:33

  • With the new dumping logic, solid element types are expected to expose four_c_element_data (and rigid spheres appear to be handled via type == "RIGIDSPHERE"). SolidRigidSphere currently doesn't define four_c_element_data, so dumping/importing rigid spheres will fail. Add a class attribute like four_c_element_data = {"type": "RIGIDSPHERE"} (and ensure material handling stays consistent with the new get_four_c_element_data() rules).
    src/beamme/mesh_creation_functions/nurbs_generic.py:97
  • add_*_nurbs_to_mesh() still accepts a data argument and forwards it into the created element instance (data=data). But dumping now explicitly rejects any per-element element.data entries and requires all element data to live on the element type (four_c_element_data). This API mismatch will lead to runtime failures when callers pass data. Consider removing/deprecating the data parameter, or merging it into element_type.four_c_element_data (or a derived type) instead of storing it on the instance.
def add_splinepy_nurbs_to_mesh(
    mesh: _Mesh,
    element_type: type,
    splinepy_obj,
    *,
    material=None,
    data: dict
    | None = None,  # TODO: Check if this is really needed! Or unify this with how solid element data is processed
) -> _GeometryName:
    """Add a splinepy NURBS to the mesh.

    Args:
        mesh: Mesh that the created NURBS geometry will be added to.
        element_type: The type of element to be created.
        splinepy_obj (splinepy object): NURBS geometry created using splinepy.
        material (Material): Material for this geometry.
        data: General element data, e.g., material, formulation, ...

    Returns:
        GeometryName:
            Set with the control points that form the topology of the mesh.

            For a surface, the following information is stored:
                Vertices: 'vertex_u_min_v_min', 'vertex_u_max_v_min', 'vertex_u_min_v_max', 'vertex_u_max_v_max'
                Edges: 'line_v_min', 'line_u_max', 'line_v_max', 'line_u_min'
                Surface: 'surf'

            For a volume, the following information is stored:
                Vertices: 'vertex_u_min_v_min_w_min', 'vertex_u_max_v_min_w_min', 'vertex_u_min_v_max_w_min', 'vertex_u_max_v_max_w_min',
                        'vertex_u_min_v_min_w_max', 'vertex_u_max_v_min_w_max', 'vertex_u_min_v_max_w_max', 'vertex_u_max_v_max_w_max'
                Edges: 'line_v_min_w_min', 'line_u_max_w_min', 'line_v_max_w_min', 'line_u_min_w_min',
                    'line_u_min_v_min', 'line_u_max_v_min', 'line_u_min_v_max', 'line_u_max_v_max'
                    'line_v_min_w_max', 'line_u_max_w_max', 'line_v_max_w_max', 'line_u_min_w_max'
                Surfaces: 'surf_w_min', 'surf_w_max', 'surf_v_min', 'surf_v_max', 'surf_v_max', 'surf_u_min'
                Volume: 'vol'
    """

    # Make sure that the control points are 3D
    nurbs_cp_dim = splinepy_obj.control_points.shape[1]
    if not nurbs_cp_dim == 3:
        raise ValueError(f"Invalid control point dimension: {nurbs_cp_dim}")

    # Make sure the material is in the mesh
    mesh.add_material(material)

    # Fill control points
    control_points = [
        _ControlPoint(coord, weight[0])
        for coord, weight in zip(
            _np.asarray(splinepy_obj.control_points), _np.asarray(splinepy_obj.weights)
        )
    ]

    # Fill element
    element = element_type(
        [_np.asarray(knot_vector) for knot_vector in splinepy_obj.knot_vectors],
        _np.asarray(splinepy_obj.degrees),
        nodes=control_points,
        material=material,
        data=data,
    )

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/beamme/four_c/model_importer.py Outdated
Comment thread src/beamme/four_c/model_importer.py Outdated
Comment thread src/beamme/four_c/model_importer.py Outdated
Comment thread src/beamme/four_c/input_file_dump_item.py Outdated
Comment thread src/beamme/four_c/input_file_dump_item.py Outdated
@isteinbrecher
Copy link
Copy Markdown
Collaborator Author

I am not fully sold on this hashing thing here. Why do we need to hash the element, and on which "data" do we actually hash it? What could be the reason that we read one element 2 times? Maybe you could explain that to me:)

@davidrudlstorfer we actually don't hash the element here, but the element type and material, i.e., the things that are common for all elements in an element block. If we want to switch to more efficient data structures, we have to group all elements that have the same element formulation, parameters and material in a block. For example, all SR elements with a certain cross-section. KL elements would be in a different block.

This grouping is achieved by hashing the element type data, e.g., element formulation, number of nodes per element, integration scheme, ..., and the material together. So elements with the same hash belong to the same block in the 4C input file.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/beamme/utils/data_structures.py:39

  • dict_to_tuple_converter returns tuples that may still contain unhashable values (e.g., lists like the solid element description key GP: [3, 3]). This will raise TypeError: unhashable type: 'list' when used in hash((base_type, dict_to_tuple_converter(element_data))) during import. Consider converting lists/tuples/sets recursively (e.g., to tuples) and optionally handling other common containers (like numpy arrays) to guarantee the result is fully hashable.
    src/beamme/four_c/element_rigid_sphere.py:25
  • With the new element dumping logic (get_four_c_element_data requires type(element).four_c_element_data), SolidRigidSphere currently lacks the required four_c_element_data class attribute, so rigid sphere export will raise a ValueError. Define four_c_element_data on this class (at minimum including {"type": "RIGIDSPHERE"}) so dumping works and the material-optional rigid-sphere path can trigger.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/beamme/four_c/model_importer.py Outdated
Copy link
Copy Markdown
Collaborator

@davidrudlstorfer davidrudlstorfer left a comment

Choose a reason for hiding this comment

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

A few comments

Comment thread src/beamme/core/base_mesh_item.py Outdated
Comment thread src/beamme/four_c/input_file_dump_item.py Outdated
Comment thread src/beamme/four_c/model_importer.py Outdated
Comment thread src/beamme/four_c/element_rigid_sphere.py Outdated
Comment thread src/beamme/utils/data_structures.py
@isteinbrecher isteinbrecher temporarily deployed to cubit_secrets_trusted March 30, 2026 13:10 — with GitHub Actions Inactive
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 8 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/beamme/utils/data_structures.py
Comment thread src/beamme/four_c/model_importer.py
Comment thread src/beamme/four_c/input_file_dump_item.py
Comment thread src/beamme/four_c/input_file_dump_item.py Outdated
Comment thread src/beamme/four_c/input_file_dump_item.py Outdated
Comment thread src/beamme/core/node.py Outdated
Comment thread src/beamme/core/element.py Outdated
Comment thread src/beamme/four_c/input_file_mappings.py Outdated
@isteinbrecher isteinbrecher temporarily deployed to cubit_secrets_trusted March 30, 2026 13:17 — with GitHub Actions Inactive
@isteinbrecher isteinbrecher temporarily deployed to cubit_secrets_trusted March 30, 2026 13:33 — with GitHub Actions Inactive
@isteinbrecher
Copy link
Copy Markdown
Collaborator Author

@davidrudlstorfer this should now be ready for review. I changed the hashing of the element block data as discussed, now we only compare dictionaries.

With this PR the data structures of the elements now closely resemble the ones needed for binary output, which should make #594 a lot easier.

I am not sure, but maybe the current changes degrade performance a little bit - for legacy output. However, once we switch to optional binary output, performance is only relevant for that case anyways, since that should be a lot faster than the current output mechanism.

@isteinbrecher isteinbrecher temporarily deployed to cubit_secrets_trusted March 30, 2026 13:39 — with GitHub Actions Inactive
Copy link
Copy Markdown
Collaborator

@davidrudlstorfer davidrudlstorfer left a comment

Choose a reason for hiding this comment

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

@isteinbrecher thanks a lot for the nice cleanup :) the code and PR is now much much easier to understand than the first versions. Especially the data comparison is now really straight forward and simple to follow.

The comments are not super important, you can also directly merge:)

Comment thread src/beamme/core/base_mesh_item.py Outdated
Comment thread src/beamme/four_c/element_beam.py Outdated
Comment thread src/beamme/four_c/input_file_dump_item.py Outdated
Comment thread src/beamme/four_c/model_importer.py
Comment thread tests/beamme/utils/test_beamme_utils_data_structures.py
Comment thread src/beamme/utils/data_structures.py
Comment thread tests/conftest_result_comparison.py
@davidrudlstorfer
Copy link
Copy Markdown
Collaborator

Only things that are missing are

  • a more descriptive name for the text based / combined / legacy input
  • resolution of new fourcipp function in testing

Let's get this merged asap :)

@isteinbrecher isteinbrecher temporarily deployed to cubit_secrets_trusted April 13, 2026 11:42 — with GitHub Actions Inactive
@isteinbrecher
Copy link
Copy Markdown
Collaborator Author

Thanks for the comments @davidrudlstorfer , this should be ready for a final round of review.

Copy link
Copy Markdown
Collaborator

@davidrudlstorfer davidrudlstorfer left a comment

Choose a reason for hiding this comment

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

Thanks for the last changes, and sorry again that this took so long from my side.

Now it really looks good :) Thanks for all the effort, compared to the v1 this now looks amazing 🚀

@isteinbrecher isteinbrecher had a problem deploying to cubit_secrets_trusted April 13, 2026 11:49 — with GitHub Actions Failure
@isteinbrecher isteinbrecher merged commit 4184415 into beamme-py:main Apr 13, 2026
12 of 13 checks passed
@isteinbrecher isteinbrecher deleted the block_element_data branch April 13, 2026 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants