Skip to content

Conversation

@amgebauer
Copy link
Member

Control points needed to be consecutive anyways. Let's see whether we can enforce this for all nodes.

Related to #733

sebproell
sebproell previously approved these changes Sep 2, 2025
Copy link
Member

@sebproell sebproell left a comment

Choose a reason for hiding this comment

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

🤞

@amgebauer amgebauer force-pushed the enforce-node-ids-being-consecutive-and-starting-with-1 branch from 564f309 to 83b5cee Compare September 2, 2025 16:08
@sebproell sebproell added the breaking change Pull requests that require manual user actions after merging label Sep 2, 2025
@amgebauer amgebauer force-pushed the enforce-node-ids-being-consecutive-and-starting-with-1 branch from 83b5cee to 5eb99e0 Compare September 2, 2025 17:24
@amgebauer amgebauer marked this pull request as ready for review September 2, 2025 18:04
@amgebauer
Copy link
Member Author

I fixed all tests. I think it is worth restricting ourselves like that to make reading a mesh from any format as efficient as possible. Any thoughts on this?

@sebproell
Copy link
Member

This might break a lot of custom workflows, not sure. I totally agree that we need to move away from users having "control" over the IDs we use internally. Others are probably in a better situation to judge whether this can be merged.

@amgebauer
Copy link
Member Author

I don't think that it will break a lot of workflows. Adapted tests appeared to be manually crafted. Any script-based workflow in production likely generates contiguous point IDs anyways.

Copy link
Member

@sebproell sebproell left a comment

Choose a reason for hiding this comment

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

I see. Only xfluid, browniandyn (@davidrudlstorfer) and one spring-dashpot test needed to be adapted. Seems like this is an acceptable assumption then.

Copy link
Contributor

@maxfirmbach maxfirmbach left a comment

Choose a reason for hiding this comment

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

I'm fine with the changes. I think it's worth changing the tests, we gain more flexibility in the end related to mesh input etc.

@georghammerl
Copy link
Member

Not sure whether this is important, our grid_generator does not provide consecutive node numbers if I get the implementation right, see

nodeids[20] = nodeOffset + (ez * ny + ey + 1) * nx + ex + 1;

Node numbers are consecutive for the HEX27 layout, while for HEX20 and HEX8 some node ids are skipped.

Honestly, to me it feels like a step backwards to enforce consecutive node numbering. Depending on which tool is used to create meshes, it is not guaranteed that this requirement is fulfilled. I personally tend to provide a "node id renumberer" to clean meshes if our requirement is not fulfilled.

@amgebauer
Copy link
Member Author

Not sure whether this is important, our grid_generator does not provide consecutive node numbers if I get the implementation right, see

nodeids[20] = nodeOffset + (ez * ny + ey + 1) * nx + ex + 1;

Node numbers are consecutive for the HEX27 layout, while for HEX20 and HEX8 some node ids are skipped.

The node ids of our grid generator are purely internal. So we can just make them consecutive.

Honestly, to me it feels like a step backwards to enforce consecutive node numbering. Depending on which tool is used to create meshes, it is not guaranteed that this requirement is fulfilled. I personally tend to provide a "node id renumberer" to clean meshes if our requirement is not fulfilled.

Most mesh formats don't even allow to give points an id. And I think we should move in that direction, i.e., removing the id completely from the input. This is an intermediate step enforcing that we don't allow doing weird things with the id while still being (mostly) compatible with the original input.

Don't get me wrong, it is super easy to also allow specifying the id from the outside, but I don't know in which scenario this simplifies things.

@amgebauer
Copy link
Member Author

Let me add a little background: We want to unify our mesh input so that every format goes through the same interface. Currently, we basically have 3 mechanisms: dat-style input, vtu/ensight and grid generator. It would be nice if we can merge all three into one so that creating a mesh is more or less just creating the object MeshInput::Mesh.

Currently we use a lot of std::map for our discretization to store nodes and elements. If we can replace these with std::vector, the underlying memory is much more flat (and if we have consecutive ids, the index is the id, so accessing elements by their id is super fast).

@georghammerl
Copy link
Member

Most mesh formats don't even allow to give points an id. And I think we should move in that direction, i.e., removing the id completely from the input. This is an intermediate step enforcing that we don't allow doing weird things with the id while still being (mostly) compatible with the original input.

I work a lot with Ansys, Nastran and Abaqus mesh files. All files that I have seen so far include node numbers. I see an import of those file formats as an important entrance step into 4C (for potential future users). Hence, it would be of importance to take care of node numbers.

I get the intention of your idea with using the more performant std::vector access. In the end it boils down to performance vs. flexibility. I tend not to loose the latter.

@mayrmt
Copy link
Member

mayrmt commented Sep 3, 2025

Most mesh formats don't even allow to give points an id.

I do not understand how you then identify nodes to assign them to elements and build up a mesh connectivity?

@amgebauer
Copy link
Member Author

@mayrmt

Most mesh formats don't even allow to give points an id.

I do not understand how you then identify nodes to assign them to elements and build up a mesh connectivity?

<?xml version="1.0"?>
<VTKFile type="UnstructuredGrid" version="0.1" byte_order="LittleEndian">
  <UnstructuredGrid>
    <Piece NumberOfPoints="8" NumberOfCells="1">
      <Points>
        <DataArray type="Float32" NumberOfComponents="3" format="ascii">
          0 0 0
          1 0 0
          1 1 0
          0 1 0
          0 0 1
          1 0 1
          1 1 1
          0 1 1
        </DataArray>
      </Points>
      <Cells>
        <DataArray type="Int32" Name="connectivity" format="ascii">
          0 1 2 3 4 5 6 7
        </DataArray>
        <DataArray type="Int32" Name="offsets" format="ascii">
          8
        </DataArray>
        <DataArray type="UInt8" Name="types" format="ascii">
          12
        </DataArray>
      </Cells>
    </Piece>
  </UnstructuredGrid>
</VTKFile>

Here is a simple vtu file (a single hex element). You cannot define the id of a point (node). You refer to it with its position in the list.

@georghammerl

I work a lot with Ansys, Nastran and Abaqus mesh files. All files that I have seen so far include node numbers. I see an import of those file formats as an important entrance step into 4C (for potential future users). Hence, it would be of importance to take care of node numbers.

The question is what we want to do with the ids. If we are only using it internally, we can just ignore any non-contiguous node-ids from the ansys mesh and go with our own id ordering. Then it is a pure implementation detail.

What do you have in mind where it is important that the node-ids in 4C must match with the ones provided in the mesh?

@georghammerl
Copy link
Member

What do you have in mind where it is important that the node-ids in 4C must match with the ones provided in the mesh?

For example, according to the FKM guideline, stresses at the component's surface as well as stress gradients into the solid are needed. That means the user would likely write a script to extract values at the (known) node numbers, in order to perform further calculations. It would be cumbersome if the user would need to work with spatial positions in order to extract relevant values in case he/she cannot rely on the node numbers to be in original order.

@georghammerl
Copy link
Member

If we switch internally to consecutive node numbering, this is fine for me because it is hidden to the user. I think it is only important that a user can access all the simulation data with the originally provided node ids (and additionally any (error) output needs to use the original numbering). In other words: Any interaction of the user with the code/results should rely on the original numbering.

@mayrmt
Copy link
Member

mayrmt commented Sep 3, 2025

I work a lot with Ansys, Nastran and Abaqus mesh files. All files that I have seen so far include node numbers.

Same applies to the standardized ExodusII format, that we use in 4C, as far as I know.

@amgebauer
Copy link
Member Author

Same applies to the standardized ExodusII format, that we use in 4C.

We have actually never used it and no-one complained so far. Only because a mesh format provides the id does not necessarily mean that we also use it.

If there are use-cases, of course, we can do that. Obviously in the 20 years history we also did not care very much about the ids. Printing the ids was never consistent because sometimes we print the raw internal ids and sometimes we translate it to the original ids (they are off by 1, fixing it is not straigt forward, see #733).

If we at some point can do adaptive mesh refinement, we probably care even less about the node ids.

Also note, that some discretizations already now enforce this (NURBS). So we are even inconsistent.

@mayrmt
Copy link
Member

mayrmt commented Sep 3, 2025

@amgebauer I'm fine with merging. Just wanted to add a format, that we use, to @georghammerl's list.

@sebproell
Copy link
Member

Sounds like:

  • internally we want to freely renumber as we see fit, especially contiguously for good data layout
  • but we need to remember the numbering that was given by users (if it is different)

I agree with both points. For now, all meshes are continuously numbered so we can rework some internals without losing anything in terms of functionality. If meshes with different numbering start to appear, we can implement a renumbering that you can query anytime you interact with user-facing errors, info, output etc. Does that sound about right?

@mayrmt
Copy link
Member

mayrmt commented Sep 4, 2025

Thanks for the summary! Sounds good to me.

@ischeider
Copy link
Contributor

Sorry, I just noticed this PR. When having meshes from various sources, they may have arbitrary node numbering, also noncontinuous, and starting from arbitrary values. How do we deal with these? If read by meshio, the renumbering is done automatically (including node sets), same has to be done (manually) with vtu, if I am not mistaken. It seems that our input file options always will use node lists (not dicts), so that the PR is a logical consequence. However, users might be happy, if they find their original node numbers in the post processor.

@amgebauer
Copy link
Member Author

As discussed, we keep the info of the original ids within 4C and don't discard them during input. We might use different ids internally if that makes things easier for us, but we have a mapping from our internal ids to the ids for the output. Closing.

@amgebauer amgebauer closed this Sep 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Pull requests that require manual user actions after merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants