clarity of types: define type rules

[ragusa]

For example, too many size_t.
Also, double vs real (float32 vs float64 or 32 vs 64 bits)

Need deeper thoughts (define type rules), followed by a refactor to clean this up.

Think of a configure option.

[andrsd]

Regarding the floating point precision: Typically this is done via a configure option. We can create an enum in the cmake script for single and double precision. We want an enum, so people do not have to type and just cycle through the options if they use ccmake (we already have this for the build type). Then, we will have cmake create a config file via configure_file. This file will define something like a Real type which will be used in the code.

Relevant bits would look like this:

CMakeLists.txt:

set(BuildValues "single;double")
if(NOT OPENSN_PRECISION)
    set(OPENSN_PRECISION "double" CACHE STRING "Floating-point precision" FORCE)
endif()
set_property(CACHE OPENSN_PRECISION PROPERTY STRINGS ${BuildValues})
...
if (OPENSN_PRECISION STREQUAL "single")
    set(OPENSN_PRECISION_TYPE "float")
elseif(OPENSN_PRECISION STREQUAL "double")
    set(OPENSN_PRECISION_TYPE "double")
else()
    message(FATAL_ERROR "Unsupported precision type: " ${OPENSN_PRECISION})
endif()
...
configure_file(config.h.in config.h)

config.h.in:

#pragma once

namespace opensn {
using Real = @OPENSN_PRECISION_TYPE@;
}

Similar approach can be used for various indices. In libMesh, they have things like dof_id_type for indexing into global solution vectors, boudary_id_type, subdomain_id_type, etc. PETSc has only PetscReal and PetscInt, IIRC. I Definitely see this happening for integers.

A lot of times I have seen this done via macros, but we should avoid those and use real C++ types as shown above...

Note that the names above are picked just for demonstration, the final names could change.

[andrsd]

Here is some insight into the craziness that is going on with integer types:

Type	Current type used	Proposed change	Note
Cell ID (global)	uint64_t	uint64_t
Cell ID (local)	uint32_t	uint64_t
✅ Vertex ID	uint64_t
✅ Partition ID	uint64_t	int	Represented as int in MPI
✅ Block ID (was material ID)	int	unsigned int
Boundary ID	uint64_t
✅ Verbosity level	int	unsigned int
LBS group ID	int	unsigned int
Number of group	size_t		groups are stored in std::vector
✅ Scattering order	size_t	unsigned int
	unsigned int		need unification
Num moments	std::size_t
Num precursors	size_t		stored in std::vector
Num DOFs	int64_t		Because PETSc uses signed type and we configure with 64 bits (technically breaks with 32 bit PETSc).
Num DOFs	size_t		Should be unified
✅ Return code from PETSc	int	PetscErrorCode	Because that is the right type
Number of iterations (PETSc)	int		Even though unsigned int is more correct, this may stay signed, because PETSc uses a signed type.
shape function index	uint32_t int		Should be unified
local side index	uint32_t
local face index	uint32_t size_t		Should be unified
quadrature point index	uint32_t
number of nodes (on a cell)	size_t
unknown id	unsigned int
component index of a unknown	unsigned int
ell	unsigned int	int
FAS	int	uint32_t

Quadrature

Thing	Current type	Proposed change	Note
n_polar	int	unsigned int
n_azimuthal	int	unsigned int
scattering order	int	unsigned int
	unsigned int		Should be unified
order	int	unsigned int
dimension	int	unsigned int	to match dimension in other places

Notes

• Should the objects that use PETSc use PETSc-defined types? Or should this be aliased?
• Legendre API uses int but there are no Legendre functions with negative index

Variables used in for-loops

loops like:

for (int i = 0; i < 4; ++i)

are ok. 0 and 4 are considered int

Loops like:

for (std::size_t i = 0; i < container.size(); ++i)

are ok, but sometimes people use int, which they should not.
This can be done via something like:

for (auto i : make_range(N))

which is will always use the right type.

Proposed changes

Most of the things can be just int or unsigned int (unsigned is probably good to use if we wanted to communicate that the thing cannot be negative - like spatial dimension, function index, etc.). However, if we wanted to be super-simple almost everything can be just int (number of groups, scattering order, you name it). It is just what you guys prefer.

Many things are stored in std::vector or some other std container and this is why we see std::size_t, however that may be overkill. Is there going to be 2Bn energy groups? But, the question still hold: "is the narrowing conversion justified?". And should we reflect on the signedness i.e. should we use int vs unsigned int where container.size() is returned via API. I think clang-tidy would be complaining, so unsigned int?

CAUTION However, things like cell ID, vertex ID, dof index etc. cannot be just int or just unsigned int (those are 32 bits numbers on both my 64-bit machines). And since we require 64-bit PETSc build those types (at least for the DOF index) must be 64 bit. Technically, the correct type is PetscInt, but not all solvers are PETSc-based, so probably uint64_t unless we want to have an alias like DofType/DofInt/... But then, DofType is not CellID, etc.

[andrsd]

I found one warning from clang-tidy triggered by concurrency-mt-unsafe. Since we are not doing threads, this should probably be removed from the clang-tidy profile.

[ragusa]

my feeling:

• the biggest discussion is int vs unsigned int : I would go with unsigned int; for example, everything listed above in the quadrature should be unsigned int
• whatever needs to be uint64_t should remain that way. I do not understand why partition ID needs to be of that type. Maybe because of Petsc.
• i could entertain the removal of size_t
• why would shape function index need to be uint32_t?

[andrsd]

I do not understand why partition ID needs to be of that type. Maybe because of Petsc.

Because that's an MPI rank and that is represented as int in MPI.

[andrsd]

i could entertain the removal of size_t

size_t is returned by size() in C++ containers like vector, etc., which is used a lot so, that will force a lot of static_casts I think.

[andrsd]

why would shape function index need to be uint32_t?

Are there shape functions with negative local indices? To me they should be unsigned if one wants to communicate that fact... But, either one might work...

[andrsd]

I do not understand why partition ID needs to be of that type. Maybe because of Petsc.

Because that's an MPI rank and that is represented as int in MPI.

I updated the table to reflect this