Skip to content

Fix uninitialized variable bug and drop dead code#44

Open
aaraney wants to merge 2 commits into
NOAA-OWP:masterfrom
aaraney:uninitialized-and-dead-code-removal
Open

Fix uninitialized variable bug and drop dead code#44
aaraney wants to merge 2 commits into
NOAA-OWP:masterfrom
aaraney:uninitialized-and-dead-code-removal

Conversation

@aaraney
Copy link
Copy Markdown
Member

@aaraney aaraney commented Aug 6, 2025

Fixes a bug in the actual water vapor partial pressure calculation caused by an uninitialized variable and drop some dead code. Reviewers are recommended to review each commit in isolation as i've split things up to make your life easier.

I one question about the value we should use for uninitialized saturation_water_vapor_partial_pressure_Pa variable in the actual water vapor partial pressure calculation that i've marked with a comment in the PR.

Removals

  • Drop unused variables in ET methods

@aaraney aaraney added the bug Something isn't working label Aug 6, 2025
@aaraney aaraney force-pushed the uninitialized-and-dead-code-removal branch 2 times, most recently from fcaf64a to b5aeca9 Compare August 6, 2025 18:42
Comment thread include/pet_tools.h
@aaraney aaraney changed the title Fix uninitialized variables and drop dead code Fix uninitialized variable bug and drop dead code Aug 6, 2025
@seyounger
Copy link
Copy Markdown
Contributor

seyounger commented Dec 16, 2025

@aaraney The saturation_water_vapor_partial_pressure_Pa is only needed when yes_aorc==0 to calculate longwave radiation, because AORC provides longwave radiation. One way to fix the uinitialized saturation_water_vapor_partial_pressure_Pa is to move the saturation_water_vapor_partial_pressure_Pa and actual_water_vapor_partial_pressure_Pa calculations inside the if block where longwave radiation is calculated.

diff --git a/include/pet_tools.h b/include/pet_tools.h
index 7ebb1c2..7bf9354 100644
--- a/include/pet_tools.h
+++ b/include/pet_tools.h
@@ -64,14 +64,12 @@ double calculate_net_radiation_W_per_sq_m(pet_model *model)
   }
   
   if(model->pet_options.yes_aorc==0)  // we must calculate longwave incoming from the atmosphere 
+  {
     saturation_water_vapor_partial_pressure_Pa=calc_air_saturation_vapor_pressure_Pa(model->surf_rad_forcing.air_temperature_C); 
 
   actual_water_vapor_partial_pressure_Pa=model->surf_rad_forcing.relative_humidity_percent/100.0*
-                                         // FIXME: REVIEWER: what should the default value be?
                                          saturation_water_vapor_partial_pressure_Pa;
 
-  if(model->pet_options.yes_aorc==0)
-  {
     // CALCULATE DOWNWELLING LONGWAVE RADIATION FLUX FROM ATMOSPHERE, W/m2.
     if(0.90 < model->surf_rad_forcing.cloud_cover_fraction) // very nearly overcast or overcast
     {

@aaraney aaraney force-pushed the uninitialized-and-dead-code-removal branch from b5aeca9 to ded0417 Compare December 17, 2025 15:23
Co-authored-by: seyounger <seth.younger@noaa.gov>
@aaraney aaraney force-pushed the uninitialized-and-dead-code-removal branch from ded0417 to 900f218 Compare December 17, 2025 15:25
Copy link
Copy Markdown
Contributor

@seyounger seyounger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Thanks @aaraney!

@aaraney
Copy link
Copy Markdown
Member Author

aaraney commented Dec 22, 2025

Preface, the CI failures are not related to this PR.

Failing test_surfacebmi_plus_cfe (macos-latest):: Build SurfaceBMI Output
Prepare all required actions
Run ./.github/actions/ngen-submod-build
Run git submodule update --init --recursive --  extern/noah-owp-modular/
Submodule 'extern/noah-owp-modular/noah-owp-modular' (https://github.com/NOAA-OWP/noah-owp-modular) registered for path 'extern/noah-owp-modular/noah-owp-modular'
Cloning into '/Users/runner/work/evapotranspiration/evapotranspiration/extern/noah-owp-modular/noah-owp-modular'...
Submodule path 'extern/noah-owp-modular/noah-owp-modular': checked out '30d0f53e8c14acc4ce74018e06ff7c9410ecc13c'
Run # see macOS image readmes for supported versions
Run cmake -B  extern/noah-owp-modular//cmake_build -S  extern/noah-owp-modular/ -DNGEN_IS_MAIN_PROJECT:BOOL=ON
CMake Error at /opt/homebrew/share/cmake/Modules/CMakeDetermineFortranCompiler.cmake:33 (message):
-- Configuring incomplete, errors occurred!
  Could not find compiler set in environment variable FC:

  gfortran-12.
Call Stack (most recent call first):
  CMakeLists.txt:11 (project)


CMake Error: CMAKE_Fortran_COMPILER not set, after EnableLanguage

The test_surfacebmi_plus_cfe (macos-latest) GH action is failing b.c. gfortran-12 is not installed in the runner's macos-latest image. As of today, macos-latest refers to macOS 15 arm64 (pinned table of image names and software). The lowest gfortran version available in that image is gfortran-13.

An interaction between the ngen-submod-build GH action template (from NOAA-OWP/ngen/.github/actions/ngen-submod-build/action.yaml) and evapotranspiration's Ngen Integration Tests is causing this failure. ngen-submod-build exports FC=gfortran-12 and Ngen Integration Tests uses the macos-latest image.

Potential Solutions

The unfortunate reality is gfortran-13 is not available by default on ubuntu-22.04. Otherwise, moving to gfortran-13 would be trivial.

Image Name Label gfortran installed
Ubuntu 24.04 ubuntu-latest 12.4.0, 13.3.0, 14.2.0
Ubuntu 22.04 ubuntu-22.04 10.5.0, 11.4.0, 12.3.0
macOS 15 macos-15-intel 13, 14, 15
macOS 15 Arm64 macos-latest 13, 14, 15
macOS 14 macos-14-large 13, 14, 15
macOS 14 Arm64 macos-14 13, 14, 15

There are several solutions:

  • Bump to GH actions across the org using the ngen-submod-build action template from Ubuntu 22.04 to Ubuntu 24.04. Then use a later version of gfortran across the board. Maybe gfortran-14? More work, but probably the right thing to do.
  • Don't specify gfortran version in ngen-submod-build GH action template. Downside is GH actions could use different gfortran versions. This will make debugging more difficult.
  • Make macOS workflows install gfortran-12. It looks like you can get gfortran-12 via brew's gcc@12 Formulae. This is the easiest way forward but adds code that doesn't really need to exist and could further complicate things in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants