Fix #10847 - EnergyPlus simulation crashes above a certain BaseDepth value for GroundHeatTransfer #10850

jmarrec · 2024-12-10T12:49:28Z

Pull request overview

Fixes EnergyPlus simulation crashes above a certain BaseDepth value for GroundHeatTransfer #10847

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Reviewer

This will not be exhaustively relevant to every PR.

fixup! Add a ctest for #10847

… this is a Debug build I don't see much of a difference to be honest here, but this seems like a logical thing

…utput

ZFACE is -35:100, positive indicates Below ground (NZBG). A, B, C,R and X are used in the 1 to NZBG range, so they should be 100 elements not 50 (Note: they are 1-indexed)

jmarrec · 2024-12-10T12:50:09Z

src/Basement/tests/AutoGriddingDepthIsLowish.idf

+EquivAutoGrid,
+  15,                                     !- CLEARANCE: Distance from outside of wall to edge of 3-D ground domain {m}
+  0.25,                                   !- SlabDepth: Thickness of the floor slab {m}
+  2.4;                                    !- BaseDepth: Depth of the basement wall below grade {m}


Add a test for a "regular" case when depth is lowish.

jmarrec · 2024-12-10T12:50:43Z

src/Basement/tests/AutoGriddingDepthIsHigh.idf

+EquivAutoGrid,
+  15,                                     !- CLEARANCE: Distance from outside of wall to edge of 3-D ground domain {m}
+  0.25,                                   !- SlabDepth: Thickness of the floor slab {m}
+  10.3;                                   !- BaseDepth: Depth of the basement wall below grade {m}


Here test with depth is 10.3, which exhibits the crash

2901: Test command: /home/julien/Software/Others/EnergyPlus-build/Products/Basement 2901: Working Directory: /home/julien/Software/Others/EnergyPlus-build/src/Basement/tst/AutoGriddingDepthIsHigh 2901: Environment variables: 2901: CI_BAIL_EARLY=1 2901: Test timeout computed to be: 10000000 2901: Completed Reading Weather File 2901: Done Parsing The Energyplus Weather File 2901: 2901: You have selected to use an equivalent foundation based on the 2901: area to perimeter ratio supplied in the IDF file. 2901: 2901: 2901: You have selected to have the solution grid sized automatically. 2901: 2901: YZ Wall TS Inside YZ Heat flux Base T 2901: XZ Wall TS Inside XZ Heat flux Base T 2901: Floor TS Inside Floor heat flux Base T 2901: YZ CL XZ CL FX CL FY CL 2901: TSYZUp TSYZUpIn TSYZLo TSYZLoIn Upper q Lower q Base T 2901: TSXZUp TSXZUpIn TSXZLo TSXZLoIn Upper q Lower q Base T 2901: TFPerim TFPerimIn TFCore TFCoreIn Perim q Core q Base T 2901: 2901: Program received signal SIGBUS: Access to an undefined portion of a memory object. 2901: 2901: Backtrace for this error: 2901: #0 0x5678a0 in ??? 2901: #1 0x566ed5 in ??? 2901: #2 0x5bc5df in ??? 2901: #3 0x41f283 in __base3d_MOD_calctearth 2901: at /home/julien/Software/Others/EnergyPlus/src/Basement/3DBasementHT.f90:9875 2901: #4 0x42e6e4 in __base3d_MOD_basementsimulator 2901: at /home/julien/Software/Others/EnergyPlus/src/Basement/3DBasementHT.f90:2587 2901: #5 0x52218b in __base3d_MOD_simcontroller 2901: at /home/julien/Software/Others/EnergyPlus/src/Basement/3DBasementHT.f90:812 2901: #6 0x5221b9 in __base3d_MOD_base3ddriver 2901: at /home/julien/Software/Others/EnergyPlus/src/Basement/3DBasementHT.f90:731 2901: #7 0x5221ec in basementmodel 2901: at /home/julien/Software/Others/EnergyPlus/src/Basement/3DBasementHT.f90:13626 2901: #8 0x52227d in main 2901: at /home/julien/Software/Others/EnergyPlus/src/Basement/3DBasementHT.f90:13620 1/2 Test #2901: Basement.AutoGriddingDepthIsHigh .....Bus error***Exception: 1.48 sec test 2902 Start 2902: Basement.AutoGriddingDepthIsLowish

jmarrec · 2024-12-10T12:52:04Z

src/Basement/3DBasementHT.f90

+      CALL GET_ENVIRONMENT_VARIABLE("CI_BAIL_EARLY", status=CI_BAIL_EARLY_STATUS)
+      IF (CI_BAIL_EARLY_STATUS == 0) THEN
+        print *, 'Exiting early because envionment variable CI_BAIL_EARLY was found'
+        CALL EXIT(0)
+      END IF


Add an environment variable "CI_BAIL_EARLY": if defined, exit after CalcTearth & PrelimOutput, which is enough to produce the crash. I don't want to add a Basement test that takes like 10 minutes, or worse like an hour (when depth is 10.3, it's looooong)

jmarrec · 2024-12-10T12:52:20Z

src/Basement/CMakeLists.txt

+if(BUILD_TESTING)
+
+  macro(basement_test GHT_IN_IDF_NAME)
+    set(TEST_DIR "${PROJECT_BINARY_DIR}/tst/${GHT_IN_IDF_NAME}") # build/src/Basement/tst
+    set(IDF_FILE "${PROJECT_SOURCE_DIR}/tests/${GHT_IN_IDF_NAME}.idf")
+
+    file(MAKE_DIRECTORY ${TEST_DIR})
+    file(COPY "${PROJECT_SOURCE_DIR}/../../idd/BasementGHT.idd" DESTINATION "${TEST_DIR}")
+    configure_file("${PROJECT_SOURCE_DIR}/../../weather/USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw" "${TEST_DIR}/in.epw" COPYONLY)
+    configure_file(${IDF_FILE} "${TEST_DIR}/BasementGHTIn.idf" COPYONLY)
+
+    set(TEST_NAME "Basement.${GHT_IN_IDF_NAME}")
+    add_test(NAME "${TEST_NAME}"
+      COMMAND $<TARGET_FILE:Basement>
+      WORKING_DIRECTORY ${TEST_DIR}
+    )
+    set_tests_properties("${TEST_NAME}" PROPERTIES
+      FAIL_REGULAR_EXPRESSION "Terminated;Error(s) Detected"
+      ENVIRONMENT "CI_BAIL_EARLY=1"
+    )
+  endmacro()
+
+  basement_test(AutoGriddingDepthIsHigh)
+  basement_test(AutoGriddingDepthIsLowish)
+
+endif()


Register the basement tests as ctest

jmarrec · 2024-12-10T12:52:53Z

cmake/Fortran.cmake

  else()
    if(NOT "Ninja" STREQUAL ${CMAKE_GENERATOR})
      target_compile_options(fortran_project_options INTERFACE -cpp)
    endif()
    set(FORTRAN_STATIC_EXE TRUE)
    target_link_options(fortran_project_options INTERFACE -static)
+    target_compile_options(fortran_project_options INTERFACE $<$<CONFIG:Debug>:-fbacktrace -ffpe-trap=zero,overflow,underflow>)


Pass some debug options to gfortran / ifort when building debug. I havern't found much differences but it makes sense

jmarrec · 2024-12-10T12:53:26Z

src/Basement/CMakeLists.txt

@@ -59,7 +59,7 @@ set(CMAKE_SKIP_RPATH ${FORTRAN_SKIP_RPATH})
 set(SKIP_BUILD_RPATH TRUE)

 add_executable(Basement ${SRC})
-target_compile_options(Basement PRIVATE -O1)
+target_compile_options(Basement PRIVATE $<$<CONFIG:Release>:-O1>)


THIS is especially needed to get the segfault.

When building debug, do NOT ask for -O1 (the flags would be -g -O1 which is weird)

jmarrec · 2024-12-10T12:53:58Z

src/Basement/3DBasementHT.f90

@@ -9634,7 +9641,7 @@ SUBROUTINE CalcTearth(IEXT,JEXT,DZ,DZP,TG,CVG)


 !*** DECLARATIONS:
-     REAL(r64) A(50), B(50), C(50), R(50), X(50), ALB, ALBEDO(2),             &
+     REAL(r64) A(100), B(100), C(100), R(100), X(100), ALB, ALBEDO(2),             &


Actual fix is here

ZFACE is ZFACE(-35:100), positive indicates Below ground (NZBG).
A, B, C,R and X are used in the 1 to NZBG range, so they should be 100 elements not 50 (Note: they are 1-indexed)

I think the -35:100 means there are 136 array elements?

https://www-eio.upc.es/lceio/manuals/Fortran95-manual.pdf

All the following type declaration statements are legal: real, dimension (-10:8) :: a1 ! 1-dim array with 19 elements integer, dimension (-3:3, -20:0, 1:2, 6, 2, 5:6, 2) :: grid1 ! 7-dim array The number of elements of the integer array grid1 is 7 x 21 x 2 x 6 x 2 x 2 x 2 = 14112.

But NZBG is a user input and I think the idd is suggesting a value of 10 to 35 for that input, so that's where the 50 came from? There is no upper limit on that field so a user could enter 150 and crash the program again.

GroundHeatTransfer:Basement:ManualGrid, \memo Manual Grid only necessary using manual gridding (not recommended) N1, \field NX: Number of cells in the X direction: 20] \minimum 1 \required-field N2, \field NY: Number of cells in the Y direction: 20] \minimum 1 \required-field N3, \field NZAG: Number of cells in the Z direction. above grade: 4 Always] \minimum 1 \required-field N4, \field NZBG: Number of cells in Z direction. below grade: 10-35] \minimum 1 \required-field NumNums=GetNumObjectsFound('ManualGrid') IF (NumNums > 0) THEN CALL GetObjectItem('ManualGrid',NUM,AlphArray,NumAlphas,NumArray,NumNums,IOSTAT) NX =NumArray(1) NY =NumArray(2) NZAG =NumArray(3) NZBG =NumArray(4)

ManualGrid is broken, there is ZERO way to use it (AUTOGRID is set to TRUE and it's never ressetable to FALSE):

EnergyPlus/src/Basement/3DBasementHT.f90

Line 999 in acda84e

AUTOGRID = 'TRUE'

I brought it up with @Myoldmopar, not sure if it was worth fixing (the Documentation explicitly says this is discouraged anyways).

The EquivAutoGrid BaseDepth is the one that determines the NZBG, and I agree I should probably do a bounds check there.

There are 136 elements in ZFACE, 35 < 0, the 0, and 100> 0 (1 to 100).

If you look closely at the routine, you'll see the A, B etc arrays are only used in the 1 to NZBG range, so it needs 100 elements.

The reason I'm not just adding an IDD bound for BaseDepth in EquivAutogrid is that the error would be cryptic versus specifically explaining why with a fatal

nrel-bot-2 · 2024-12-21T00:17:34Z

@jmarrec it has been 10 days since this pull request was last updated.

nrel-bot-2 · 2024-12-28T00:21:25Z

@jmarrec it has been 7 days since this pull request was last updated.

jmarrec added 6 commits December 10, 2024 13:42

Add a ctest for #10847

2ea13f0

fixup! Add a ctest for #10847

do NOT link Basement with -O1 when this is not a release build

b0ef522

Fortran: Pass -ftraceback and -ffpe-trap=zero,overflow,underflow when…

566991c

… this is a Debug build I don't see much of a difference to be honest here, but this seems like a logical thing

Add a working test with an AutoGrid that has a depth that isn't too high

f8f3134

Add a CI_BAIL_EARLY env variable to stop after CalcTearth and PrelimO…

33cecf6

…utput

Fix #10847 - allocate right sized arrays

8c6410a

ZFACE is -35:100, positive indicates Below ground (NZBG). A, B, C,R and X are used in the 1 to NZBG range, so they should be 100 elements not 50 (Note: they are 1-indexed)

jmarrec added Defect Includes code to repair a defect in EnergyPlus AuxiliaryTool Related to an auxiliary tool, not EnergyPlus itself (readvars, preprocessor, ep-launch, etc.) labels Dec 10, 2024

jmarrec self-assigned this Dec 10, 2024

jmarrec commented Dec 10, 2024

View reviewed changes

jmarrec mentioned this pull request Dec 10, 2024

EnergyPlus simulation crashes above a certain BaseDepth value for GroundHeatTransfer #10847

Open

jmarrec added 2 commits December 11, 2024 09:35

Add bounds checking for autogridding so NZBG <= 100

c946eba

Add IDD bounds for ManualGrid (can't be used) for completeness

1eeccdb

The reason I'm not just adding an IDD bound for BaseDepth in EquivAutogrid is that the error would be cryptic versus specifically explaining why with a fatal

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #10847 - EnergyPlus simulation crashes above a certain BaseDepth value for GroundHeatTransfer #10850

Fix #10847 - EnergyPlus simulation crashes above a certain BaseDepth value for GroundHeatTransfer #10850

jmarrec commented Dec 10, 2024

jmarrec Dec 10, 2024

jmarrec Dec 10, 2024

jmarrec Dec 10, 2024

jmarrec Dec 10, 2024

jmarrec Dec 10, 2024

jmarrec Dec 10, 2024

jmarrec Dec 10, 2024

rraustad Dec 10, 2024

rraustad Dec 10, 2024 •

edited

Loading

jmarrec Dec 11, 2024

nrel-bot-2 commented Dec 21, 2024

nrel-bot-2 commented Dec 28, 2024

Fix #10847 - EnergyPlus simulation crashes above a certain BaseDepth value for GroundHeatTransfer #10850

Are you sure you want to change the base?

Fix #10847 - EnergyPlus simulation crashes above a certain BaseDepth value for GroundHeatTransfer #10850

Conversation

jmarrec commented Dec 10, 2024

Pull request overview

Pull Request Author

Reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rraustad Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrel-bot-2 commented Dec 21, 2024

nrel-bot-2 commented Dec 28, 2024

rraustad Dec 10, 2024 •

edited

Loading