Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialise ssnow variables in mpiworker #471

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SeanBryan51
Copy link
Collaborator

@SeanBryan51 SeanBryan51 commented Nov 12, 2024

Variables ssnow%rtevap_sat, ssnow%rtevap_unsat and ssnow%qrecharge are uninitialised in all MPI worker processes and crashes the model when runtime memory checks are enabled (see #397 for more details). These variables are specifically initialised (to zero) in cable_parameters.F90 for the serial case (and for the MPI master process). Rather than communicating the initialised variables from master to worker, this change initialises the variables in the worker directly.

Although we plan to deprecate the MPI drivers in the future, this change will still be useful when comparing the behaviour of MPI vs serial during development.

Fixes #397

Type of change

Please delete options that are not relevant.

  • Bug fix

Checklist

  • I have checked my code/text and corrected any misspellings

📚 Documentation preview 📚: https://cable--471.org.readthedocs.build/en/471/

@SeanBryan51
Copy link
Collaborator Author

Note to reproduce the error, the following temporary patch was used to "fix" #396:

diff --git a/src/offline/cable_parameters.F90 b/src/offline/cable_parameters.F90
index 8117f3f..f9c242e 100644
--- a/src/offline/cable_parameters.F90
+++ b/src/offline/cable_parameters.F90
@@ -2294,22 +2294,22 @@ CONTAINS
   endwhere
 
   ! ____________________ MMY comment out as we don't use hys ___________________________
-  do k=1,ms
-     do i=1,mp
-        if (ssnow%wb_hys(i,k) .lt. 0._r_2) then
-           ssnow%wb_hys(i,k) = ssnow%wb(i,k)
-        end if
-        ssnow%wb_hys(i,k)  = max(soil%watr(i,k) ,min(soil%ssat_vec(i,k), ssnow%wb_hys(i,k)))
-
-       if (ssnow%smp_hys(i,k) .lt. -1.0e+30_r_2) then  !set to missing, calc
-          ssnow%smp_hys(i,k) = -soil%sucs_vec(i,k)*  &
-                               ( (ssnow%wb_hys(i,k)-ssnow%watr_hys(i,k))/&
-                                 (ssnow%ssat_hys(i,k)-ssnow%watr_hys(i,k)) )**&
-                                (-1._r_2/soil%bch_vec(i,k) )
-       end if
-       ssnow%smp_hys(i,k) = max(-1.0e10,min(-soil%sucs_vec(i,k),ssnow%smp_hys(i,k) ))
-    end do
-  end do
+!   do k=1,ms
+!      do i=1,mp
+!         if (ssnow%wb_hys(i,k) .lt. 0._r_2) then
+!            ssnow%wb_hys(i,k) = ssnow%wb(i,k)
+!         end if
+!         ssnow%wb_hys(i,k)  = max(soil%watr(i,k) ,min(soil%ssat_vec(i,k), ssnow%wb_hys(i,k)))
+
+!        if (ssnow%smp_hys(i,k) .lt. -1.0e+30_r_2) then  !set to missing, calc
+!           ssnow%smp_hys(i,k) = -soil%sucs_vec(i,k)*  &
+!                                ( (ssnow%wb_hys(i,k)-ssnow%watr_hys(i,k))/&
+!                                  (ssnow%ssat_hys(i,k)-ssnow%watr_hys(i,k)) )**&
+!                                 (-1._r_2/soil%bch_vec(i,k) )
+!        end if
+!        ssnow%smp_hys(i,k) = max(-1.0e10,min(-soil%sucs_vec(i,k),ssnow%smp_hys(i,k) ))
+!     end do
+!   end do
 
  ! MMY note that after commenting out, wb_hys and smp_hys are not defined any more
 

@SeanBryan51 SeanBryan51 force-pushed the 397-uninitialised-ssnowrtevap_sat-and-ssnowrtevap_unsat-in-worker-processes branch 2 times, most recently from 87aa062 to db04538 Compare February 11, 2025 05:38
@SeanBryan51 SeanBryan51 changed the title Initialise ssnow%rtevap_* variables in mpiworker Initialise ssnow variables in mpiworker Feb 11, 2025
@SeanBryan51 SeanBryan51 changed the title Initialise ssnow variables in mpiworker Initialise ssnow variables in mpiworker Feb 11, 2025
@SeanBryan51
Copy link
Collaborator Author

SeanBryan51 commented Feb 11, 2025

Benchcab regression tests pass for both site and spatial runs:

2025-02-11 17:25:58,521 - INFO - benchcab.benchcab.py:380 - Running comparison tasks...
2025-02-11 17:25:58,544 - INFO - benchcab.benchcab.py:381 - tasks: 168 (models: 2, sites: 42, science configurations: 4)
2025-02-11 17:29:02,317 - INFO - benchcab.benchcab.py:391 - 0 failed, 168 passed

config.yaml:

realisations:
  - repo:
      git:
        branch: main
  - repo:
      git:
        branch: 397-uninitialised-ssnowrtevap_sat-and-ssnowrtevap_unsat-in-worker-processes

modules: [
  intel-compiler/2021.1.1,
  netcdf/4.7.4,
  openmpi/4.1.0
]

@SeanBryan51 SeanBryan51 marked this pull request as ready for review February 11, 2025 22:42
@SeanBryan51 SeanBryan51 requested review from a team and abhaasgoyal and removed request for a team February 11, 2025 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uninitialised ssnow%rtevap_sat and ssnow%rtevap_unsat in worker processes
1 participant