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

Make CGNS code C++11 compliant #393

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

jrwrigh
Copy link

@jrwrigh jrwrigh commented Jul 21, 2023

This mostly consists of changing auto types to their actual types.

This also depends on #391 for me to be able to build with CGNS enabled locally (see the TEMP commit 0134d3f).

Closes: #392

- also something with references that my compiler wasn't happy about
  with regards to the `const auto &m : components` statement
 - Mostly just substituting `auto`s
@KennethEJansen
Copy link
Contributor

I have started my work from this branch.

@KennethEJansen
Copy link
Contributor

KennethEJansen commented Aug 1, 2023

@jedbrown (that is not completing so I am not sure how Jed was on the earlier CGNS ticket but not here) contributed this picture in CNDA Slack and in the prior CGNS issue #296 that has been closed. From cgnsview

With the following description (that I will lace my questions into as I try to collect the data to throw at these nodes, I am looking for a bit more detail about what is needed in each node/leaf).

  • One contiguous GridCoordinates, which gives a unique global ID to each node clear

  • TetElements followed by PyramidElements followed by PrismElements cover the volume. There are two entries here. ElementRange and ElementConnectivity. The former seems to need to be continuous, starting from from 1 for a given zone and continue as you move to a new topology of a given zone. Obviously we can can create this numbering but I don't think SCOREC/core will spit them out of a single iterator like this but with multiple iterators and a conditional and counter it should not be hard. As I look at the documentation, it shows ElementType_t between ElementRange and ElementConnectivity. Does cgnsview just not show this or, it it promoting that to the foldername? I think no because it carries number of nodes (e.g., Hexa_27). ElementConnectivity is a table (all nodes of an element per row).

This example has 3 element groups in a single zone: PyramidElements, TetElements, and Prism Elements

What FSFBrackets are right after PrismElements and before ZoneBC? Is this just a boundary element mesh set that got written before before the ZoneBC after which the rest? Was this by design or a glitch to the organization?
ZoneBC is next but the description skips to...

  • Each semantic surface (like Nacelle here) has a Elements_t node at the same level (under blk-1) with the element connectivity. This is a Pointwise convention, and it would also be possible to lump all the triangle faces into a single file node. If I understand this correctly, these are a single element group for each model face following the same convention as the volume elements above. Specifically, I think the first set will range from 1 to the number of mesh faces on the first model face, followed by ElementType, followed by the connectivity table.

  • Under ZoneBC, there is a BC_t node for each semantic surface, pointing to the element range (which is contiguous because of the Pointwise convention, but need not be) Do we need these or are we going to derive all boundary conditions from model face mesh sets in the prior paragraph?

  • For each BC, there is also a Family_t node under Base that can offer more semantic information about that boundary type. The screenshot here doesn't use that in a meaningful way. Same question?

  • Note that Symmetry contains both triangles and quads, so there are two Elements_t nodes in the file, tri_Symmetry and quad_Symmetry, with element numbering contiguous across the two. The corresponding BC_t Symmetry node refers to the contiguous range over both face topologies. Clear.

  • Note that you generally don't manage writing these file nodes manually, but rather through the Boundary Condition interfaces. @jrwrigh we should find a time to discuss this. I am currently reviewing what chef does to find the places we can extract this data from the mds database but I assume to use these functions we will have to put it into the specific data structures that these interfaces require. If we get that right your work might be pretty easy.

  • To answer your question, yes this is more of the "mesh sets" model. In PETSc, we'll create the volume elements and interpolate the mesh (create the faces and edges), then broker identification of the faces through a vertex (to keep communication structured and scalable) so we can add them to the appropriate Face Sets.
    OK my largest question is whether we will or will not need ZoneBC information that I have skipped above (thinking we don't need it). Second question is how we plan to handle periodicity. I think Jed discussed that somewhere so I will find that and surface that in another comment.

@KennethEJansen
Copy link
Contributor

KennethEJansen commented Aug 1, 2023

Notes: (see CPEX 0031)
The use of ElementList and ElementRange for ptset_type is deprecated and should not be used in new code for writing boundary conditions. These are still currently accepted, but will be internally replaced with the appropriate values of PointList/PointRange and GridLocation_t, based on the base cell dimension.
Code which reads older CGNS files, should handle ElementList and ElementRange, however, since many older files contain these specifications for ptset_type.

Is from the link @jedbrown gave to Boundary Condition interfaces. It looks like they don't want boundary conditions to be given in terms of face sets which is unfortunate in my opinion. Boundary conditions are attributes of model faces. Mesh "nodes" on model edges and model vertices are best resolved by inheritance rules from the intersection of faces. Obviously we can do that work on the SCOREC core side and produce node lists for each boundary condition but I am not clear yet whether PETSc wants these node lists or wants to do those inheritance rules and intersection from faces lists.

Weak BCs certainly belong on faces. Are they suggesting that these also should be given as node lists?

@jedbrown
Copy link

jedbrown commented Aug 2, 2023

Regarding BCs, GridLocation_t can take the value FaceCenter, so this is just a naming thing (preferring to call it PointList with FaceCenter instead of ElementList). Notable that Pointwise is still creating files with the old convention.

@jedbrown
Copy link

jedbrown commented Aug 4, 2023

As I look at the documentation, it shows ElementType_t between ElementRange and ElementConnectivity. Does cgnsview just not show this or, it it promoting that to the foldername?

You'll see it if you click on the "folder" nodes. The 10 appearing on the right is the enum value from the source code
image

typedef enum {
  CGNS_ENUMV( ElementTypeNull  ) =CG_Null,
  CGNS_ENUMV( ElementTypeUserDefined ) =CG_UserDefined,
  CGNS_ENUMV( NODE ) =2,
  CGNS_ENUMV( BAR_2 ) =3,
  CGNS_ENUMV( BAR_3 ) =4,
  CGNS_ENUMV( TRI_3 ) =5,
  CGNS_ENUMV( TRI_6 ) =6,
  CGNS_ENUMV( QUAD_4 ) =7,
  CGNS_ENUMV( QUAD_8 ) =8,
  CGNS_ENUMV( QUAD_9 ) =9,
  CGNS_ENUMV( TETRA_4 ) =10,
  CGNS_ENUMV( TETRA_10 ) =11,
  CGNS_ENUMV( PYRA_5 ) =12,
  CGNS_ENUMV( PYRA_14 ) =13,
  CGNS_ENUMV( PENTA_6 ) =14,
  CGNS_ENUMV( PENTA_15 ) =15,
  CGNS_ENUMV( PENTA_18 ) =16,
  CGNS_ENUMV( HEXA_8 ) =17,
[...]

What FSFBrackets are right after PrismElements and before ZoneBC?

The ordering in cgnsview is arbitrary from what I can tell. It's just a surface with a few hundred triangles, and has a corresponding entry in ZoneBC.

Each semantic surface (like Nacelle here) has a Elements_t node at the same level (under blk-1) with the element connectivity. This is a Pointwise convention, and it would also be possible to lump all the triangle faces into a single file node. If I understand this correctly, these are a single element group for each model face following the same convention as the volume elements above. Specifically, I think the first set will range from 1 to the number of mesh faces on the first model face, followed by ElementType, followed by the connectivity table.

Mostly, though usually one writes the volume elements first so those start at 1 and the face numbers start after that. If you click around in this file, the element blocks are numbered sequentially with tet, pyramid, prism, then FSFBrackets (triangles), etc. The ordering here is arbitrary. Note that usually the mapping to file schema is handled by the library (we won't be reading/writing raw HDF5, but rather calling the CGNS interfaces that create these linked nodes). But I've certainly found it useful to explore using cgnsview to see how a file was intended to be used.

Under ZoneBC, there is a BC_t node for each semantic surface, pointing to the element range (which is contiguous because of the Pointwise convention, but need not be) Do we need these or are we going to derive all boundary conditions from model face mesh sets in the prior paragraph?

They're supposed to be present and will be written when using the API. Here's an example from the test suite (includes multi-zone unstructured)

image

For each BC, there is also a Family_t node under Base that can offer more semantic information about that boundary type. The screenshot here doesn't use that in a meaningful way. Same question?

This appears optional, and is not written in the unstructured test. I haven't seen an example in which it's used for something I can recognize as important.

OK my largest question is whether we will or will not need ZoneBC information that I have skipped above (thinking we don't need it).

We want that because it's the way BCs are identified. It is automatically written by the interface. If for some reason we need naming heuristics instead, we could handle that, but we may as well write compliant files.

Second question is how we plan to handle periodicity. I think Jed discussed that somewhere so I will find that and surface that in another comment.

There is cg_conn_write. The docs (excerpt quoted below) describes how one can either match vertices to donor vertices or faces to donor faces. Note that CGNS vertices are all the nodes while PETSc vertices are just corners (of arbitrary order parametric elements). So my preference is to describe periodicity in terms of faces, which can be directly fed into the isoperiodic support. Life will be easiest if those face elements are oriented the same way on the periodic and donor surfaces.

For Abutting or Abutting1to1 interfaces, GridLocation can be either Vertex or FaceCenter. When GridLocation is set to Vertex, then PointList or PointRange refer to node indices, for both structured and unstructured grids. When GridLocation is set to FaceCenter, then PointList or PointRange refer to face elements. Face elements are indexed using different methods depending if the zone is structured or unstructured.

@cwsmith
Copy link
Contributor

cwsmith commented Aug 24, 2024

I'm very late to the discussion here.

Using GCC 12.3.0 I had only one compiler warning (promoted to an error) when compiling CGNS (develop @ 36bc48e) with pumi that was fixed with this commit.

Unless there is a strong reason, I'd prefer to leave the CGNS code as is (with the small fix) and rely on C++14.

Update: The fixes @jrwrigh had added in #391 were committed to develop (outside of the PR), the variable length arrays in test/convert.cc were replaced, and a few memory issues were fixed (279c82c).

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.

4 participants