search

enzo-project / enzo-e

A version of Enzo designed for exascale and built on charm++.
Other
29 stars 35 forks source link

`NumberOfBaryonFields` ptr versus int logic #98

Closed pgrete closed 2 years ago

pgrete commented 3 years ago

After a system update (which came with an updated g++), Enzo-E now fails to compile with

build-master/Enzo/enzo_EnzoMethodPpml.cpp: In member function ‘virtual double EnzoMethodPpml::timestep(Block*) const’:
build-master/Enzo/enzo_EnzoMethodPpml.cpp:96:39: error: ordered comparison of pointer with integer zero (‘int*’ and ‘int’)
   96 |   if (EnzoBlock::NumberOfBaryonFields > 0) {
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
Fatal Error by charmc in directory /home/pgrete/src/enzo-e
   Command g++ -DCMK_GFORTRAN -I/home/pgrete/src/charm/bin/../include -D__CHARMC__=1 -DCONFIG_PRECISION_DOUBLE -DSMALL_INTS -DCONFIG_NODE_SIZE=64 -DCONFIG_NODE_SIZE_3=192 -DNO_FREETYPE -DCONFIG_USE_PERFORMANCE -DCONFIG_USE_MEMORY -DCONFIG_HAVE_VERSION_CONTROL -Iinclude -I/usr/include -I/usr/include/boost -I/lib/x86_64-linux-gnu/include -O3 -g -ffast-math -funroll-loops -fPIC -pedantic -U_FORTIFY_SOURCE -fno-stack-protector -fno-lifetime-dse -c build-master/Enzo/enzo_EnzoMethodPpml.cpp -o build-master/Enzo/enzo_EnzoMethodPpml.o returned error code 1

This also applies to other files, e.g., build-master/Enzo/enzo_SetMinimumSupport.cpp or build-master/Enzo/enzo_SolveMHDEquations.cpp.

As far as I can tell int EnzoBlock::NumberOfBaryonFields[CONFIG_NODE_SIZE]; from enzo_EnzoBlock will always be != nullptr because CONFIG_NODE_SIZE is a compile time constant. Thus, the conditional as it stands will always be true (if it compiles).

I assume that the idea behind the conditional is that those code pieces should only be called if there are actualy baryon fields. If that's true, then the check should be updated to sth like EnzoBlock::NumberOfBaryonFields[my_processing_element] > 0.

More generally, I wonder if NumberOfBaryonFields actually needs to be an array (similar to the many other variables of EnzoBlock such as the block dimensions) as those numbers are (AFAIK) constant across all PEs for a given simulation.

@jobordner it'd be great to get your input on this as I'm probably missing sth (for not being too familiar with the low level design).

mabruzzo commented 3 years ago

@pgrete, I'm pretty confident that you're right; we should probably be checking the value of EnzoBlock::NumberOfBaryonFields[my_processing_element] > 0. There's also a similar check in EnzoBlock::SolveMHDEquations

Actually, looking at the code within that "if" statement, it would probably be more robust to directly check whether the fields called "density", "velox", "veloy", "veloz", "bfieldx", "bfieldy", and "bfieldz" are all defined. (As an aside, the field names should probably also be updated so that they are consistent with other solvers, but that might be kind of involved).

I don't think that anyone actually uses this solver, so I would definitely go for the "easy" fix.

jobordner commented 3 years ago

This has been addressed in PR #97

pgrete commented 3 years ago

This has been addressed in PR #97

Great! I'll link the PR so that the issue will be closed automatically once #97 is in.