mdolab / pysurf

pySurf provides geometric operations for triangulated surfaces.
Apache License 2.0
2 stars 4 forks source link

Use CGNS data types #35

Closed DavidAnderegg closed 2 months ago

DavidAnderegg commented 3 months ago

Purpose

I had problems compiling this on our cluster. Turns out, the data type for CGNS-calls is hard coded. This PR fixes this by using the data type defined by CGNS.

I only looked at places, where my compilation failed. This problem might still exist in places where my CGNS install happened to have the 'correct' data type.

Expected time until merged

1 week

Type of change

Testing

It compiles now

Checklist

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 50.11%. Comparing base (045ce73) to head (d5804a7). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #35 +/- ## ======================================= Coverage 50.11% 50.11% ======================================= Files 5 5 Lines 1688 1688 ======================================= Hits 846 846 Misses 842 842 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

eirikurj commented 3 months ago

@DavidAnderegg thanks for the PR. For documentation purposes, can you please document the compile error that you are seeing? Including the compiler type and version also helps. We are not experiencing this issue with the current compilers and versions we use. However, I am not surprised that this is showing up.

DavidAnderegg commented 3 months ago

@eirikurj sure, here is the output (I cropped the beginning):

make[2]: Entering directory `/cluster/home/software/src/MACH/pysurf_andv/src/CGNSInterface'
make -j 4 cgnsGrid.o
make[3]: Entering directory `/cluster/home/software/src/MACH/pysurf_andv/src/CGNSInterface'
mpifort -I../../mod -I/software/anaconda3/envs/g_MACH_srpl/external/cgns/include -fPIC -O2 -fdefault-real-8 -fdefault-double-8 -g -fbounds-check -std=f2008 -c cgnsGrid.F90 -o ../../obj/cgnsGrid.o

        --- Compiled cgnsGrid.F90 successfully ---

make[3]: Leaving directory `/cluster/home/software/src/MACH/pysurf_andv/src/CGNSInterface'
make -j 4 cgnsInterface.o
make[3]: Entering directory `/cluster/home/software/src/MACH/pysurf_andv/src/CGNSInterface'
mpifort -I../../mod -I/software/anaconda3/envs/g_MACH_srpl/external/cgns/include -fPIC -O2 -fdefault-real-8 -fdefault-double-8 -g -fbounds-check -std=f2008 -c cgnsInterface.F90 -o ../../obj/cgnsInterface.o
cgnsInterface.F90:352:74:

  352 |                 call cg_zone_read_f(cg, base, iZone, zoneName, dims, ierr)
      |                                                                          1
Error: Type mismatch in argument 'size' at (1); passed INTEGER(4) to INTEGER(8)
cgnsInterface.F90:372:74:

  372 |                 call cg_zone_read_f(cg, base, iZone, zoneName, dims, ierr)
      |                                                                          1
Error: Type mismatch in argument 'size' at (1); passed INTEGER(4) to INTEGER(8)
cgnsInterface.F90:418:85:

  418 |                                            type, eBeg, eEnd, nBdry, parentFlag, ierr)
      |                                                                                     1
Error: Type mismatch in argument 'start' at (1); passed INTEGER(4) to INTEGER(8)
cgnsInterface.F90:498:72:

  498 |                                                   ElementDataSize, ierr)
      |                                                                        1
Error: Type mismatch in argument 'elementdatasize' at (1); passed INTEGER(4) to INTEGER(8)
make[3]: *** [cgnsInterface.o] Error 1
make[3]: Leaving directory `/cluster/home/software/src/MACH/pysurf_andv/src/CGNSInterface'
make[2]: *** [all] Error 2
make[2]: Leaving directory `/cluster/home/software/src/MACH/pysurf_andv/src/CGNSInterface'
make[1]: *** [discretesurf] Error 1
make[1]: Leaving directory `/cluster/home/software/src/MACH/pysurf_andv'
make: *** [default] Error 1

And here are the compiler versions etc.:

(g_MACH_srpl) gcc --version
gcc (Spack GCC) 9.4.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

(g_MACH_srpl) gfortran --version
GNU Fortran (Spack GCC) 9.4.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

(g_MACH_srpl) mpirun --version
mpirun (Open MPI) 4.1.1

Report bugs to http://www.open-mpi.org/community/help/
(g_MACH_srpl) python --version
Python 3.11.7
(g_MACH_srpl)
eirikurj commented 3 months ago

Looks like cgnslib is compiled with 64-bit support. Can you confirm that? At the moment, we typically recommend 32-bit support (see here)

DavidAnderegg commented 3 months ago

Not sure how to confirm this...? As far as I understand, 64bit is opt-in, right? I dont think I opted-in. Also, other calls to CGNS would fail if it was 64bit, right?

I believe, these are the options I used to compile it:

wget https://github.com/CGNS/CGNS/archive/refs/tags/v4.2.0.tar.gz
tar -xvaf v4.2.0.tar.gz
cd CGNS-4.2.0/src
CC=mpicc CFLAGS="-ldl -fPIC" FC=mpif90 FCFLAGS="-ldl -fPIC" MPIEXEC="mpirun -n \$\${NPROCS:=4}" \
    ./configure \
    --prefix=$CONDA_PREFIX/external/cgns \
    --with-fortran  \
    --disable-cgnstools
make -j 12
make -j 12 test
make install
DavidAnderegg commented 3 months ago

@eirikurj I reinstalled everything (including CGNS and PETSC) based on the link you provide above. This time, pysurf compiles like it should. Thus I assume CGNS was compiled with 64bit support before.

I am not sure what this means regarding this PR. I believe, it is still valuable because hard-coding things that could be variable is bad practice in my opinion. Also, the changes I introduce were basically copied from ADflow.

eirikurj commented 3 months ago

Yes, ever since CGNS 4.2 64-bit is the default, so you need to manually set it to 32-bit.

I believe, it is still valuable because hard-coding things that could be variable is bad practice in my opinion.

I agree, the changes are good, just wanted to clarify. However, I think there are still other variables there that should also be changed. I think, for example, tmpConn should probably be cgsize_t. Also, nPnts, but this is less important as relates to code that is at the moment inactive (commented out). We can also fix those later, as I am sure that other issues will show up if using mixed integer precision between the code and the CGNS lib. Just let me know and we can create an issue.