Skip to content

Correct regional boundary condition indices#220

Closed
kaiyuan-cheng wants to merge 1 commit into
NOAA-GFDL:mainfrom
kaiyuan-cheng:regional_bound
Closed

Correct regional boundary condition indices#220
kaiyuan-cheng wants to merge 1 commit into
NOAA-GFDL:mainfrom
kaiyuan-cheng:regional_bound

Conversation

@kaiyuan-cheng
Copy link
Copy Markdown
Contributor

Description

Address the PR #219.

Fixes # (issue)

  1. ps_reg is initialized to signalling NaN
  2. correct indices to avoid accessing uninitialized memory

How Has This Been Tested?

Tested on Gaea and Stellar. The fix does not change the answer.

Checklist:

Please check all whether they apply or not

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@SamuelTrahanNOAA
Copy link
Copy Markdown

I'm testing this now on Jet and Hera to see if it fixes the boundary bugs.

@SamuelTrahanNOAA
Copy link
Copy Markdown

@kaiyuan-cheng These changes need to be on top of dev/emc for them to be in RRFS or UFS. Can you please open a PR to that branch?

@SamuelTrahanNOAA
Copy link
Copy Markdown

@kaiyuan-cheng The changes actually fail to compile in UFS, so I can't even test them. Also, this leaves me wondering if some of the bugs I'm seeing originate from differences between "main" and "dev/emc"

@SamuelTrahanNOAA
Copy link
Copy Markdown

I'm uncertain of what to do with this difference between the files (- lines are dev/emc, and + lines are your branch) but they do relate to indexing.

@@ -4281,9 +4307,9 @@
         enddo
       enddo
 !
-      ie=min(ubound(side_t0%w_BC,1),ubound(w,1))
-      je=min(ubound(side_t0%w_BC,2),ubound(w,2))
-      nz=ubound(w,3)
+      ie=min(ubound(side_t0%delp_BC,1),ubound(delp,1))
+      je=min(ubound(side_t0%delp_BC,2),ubound(delp,2))
+      nz=ubound(delp,3)
 !
       do nt=1,ntracers                                                                                                                                                                                                                      
         do k=1,nz                                                                                                                                                                                                                           
          do j=jstart,jend
          do i=i1,i2
            q(i,j,k,nt)=side_t0%q_BC(i,j,k,nt)                            &
                       +(side_t1%q_BC(i,j,k,nt)-side_t0%q_BC(i,j,k,nt))   &
                        *fraction_interval
          enddo
          enddo
        enddo
      enddo

@SamuelTrahanNOAA
Copy link
Copy Markdown

SamuelTrahanNOAA commented Sep 28, 2022

I also see this difference, and I don't know what to make of it. Again, - lines are dev/emc, and + lines are your branch:

-         call mappm(km, pe0, qp, npz, pe1,  qn1, is,ie, 0, 8, Atm%ptop)
+         call mappm(km, pe0, qp, npz, pe1,  qn1, is,ie, 0, 8)

This also has some indexing:

@@ -3765,11 +3787,9 @@
 ! If the source is from old GFS or operational GSM then the tracers will be fixed in the boundaries
 ! and may not provide a very good result
 !
+#ifndef SW_DYNAMICS
   if ( .not. data_source_fv3gfs ) then
-   if ( Atm%flagstruct%nwat .eq. 6 .or. Atm%flagstruct%nwat .eq. 7 ) then
-      if ( hailwat > 0 ) then
-        BC_side%q_BC(is:ie,j,1:npz,hailwat) = 0.
-      endif
- 
+   if ( Atm%flagstruct%nwat .eq. 6 ) then
       do k=1,npz
          do i=is,ie
             qn1(i,k) = BC_side%q_BC(i,j,k,liq_wat)

@SamuelTrahanNOAA
Copy link
Copy Markdown

SamuelTrahanNOAA commented Sep 28, 2022

I tried applying just the four lines you changed, to dev/emc, and the code fails here in fv_diagnostics.F90 with a "floating-point exception:"

Edit: It fails in debug mode (-DDEBUG=ON at ufs-weather-model level), not when optimized. The test used the gfortran compiler.

      do k=1,km
      do j=js,je
         do i=is,ie
!           qmin = min(qmin, q(i,j,k))                                                                                                                                                                                                       
!           qmax = max(qmax, q(i,j,k))                                                                                                                                                                                                       
            if( q(i,j,k) < qmin  ) then   ! <---- FLOATING-POINT EXCEPTION HERE
                qmin = q(i,j,k)
            elseif( q(i,j,k) > qmax ) then
                qmax = q(i,j,k)
            endif
          enddo
      enddo
      enddo

SamuelTrahanNOAA added a commit to SamuelTrahanNOAA/GFDL_atmos_cubed_sphere that referenced this pull request Oct 3, 2022
laurenchilutti pushed a commit that referenced this pull request Oct 17, 2022
…ncorrectly initialized memory. (#219)

* fixes and workarounds for uninitialized memory in fv_regional_bc

* remove workarounds and fix remaining known bugs in ps_reg

* a few more surface pressure bug fixes; now the test case runs in debug mode

* workarounds and bug fixes from gnu compiler testing

* remove -9999999 commented-out code

* quiet the NaNs passed to Atmp%ps

* simplify comments and explain snan

* use i-1 & j-1 for two-point averages, when available

* Replace many changes with PR #220
junwang-noaa pushed a commit to NOAA-EMC/GFDL_atmos_cubed_sphere that referenced this pull request Oct 21, 2022
* fixes io performance issues by making everyone a reader when io_layout=1,1
adds capability to use FMS feature to ignore data integrity checksums in restarts
* rename enforce_rst_cksum to ignore_rst_cksum and change the default value for compatibility
* updated UFS/GFS atmosphere.F90 driver as per @BinLiu-NOAA and @junwang-noaa
* Regional decomposition test fix (when nrows_blend > 0) (NOAA-GFDL#194)
* Add missing instance for hailwat
* Regional bc blend changes to extend into interior halos and overlap on corners. Still not working for u and v.
* atmosphere.F90 : add hailwat to check
dyn_core.F90 : Fix from Jun Wang to correct sync of u,v
fv_regional_bc.F90 : add check for nrows_blend > tile size; fix error when nrows_blend=1

* Explanatory comment added

* Removed commented code

* Clean old code

* In fv_fill.F90, use kind_phys for kind_phys instead of hard-coding 8 byte reals. (NOAA-GFDL#193)

* Expose remap_scalar and remap_dwinds to fv3-jedi (NOAA-GFDL#199)

* changed interface to public

* added public

* removed source

* mods for jedi build

* Transfer changes from PR NOAA-GFDL#202 to NOAA-GFDL#199

Made small changes from PR NOAA-GFDL#202 manually.

* returned ignore checksum

* fixed ignore checksum

* Fix several bugs in fv_regional_bc.F90 relating to uninitialized or incorrectly initialized memory. (NOAA-GFDL#219)

* fixes and workarounds for uninitialized memory in fv_regional_bc

* remove workarounds and fix remaining known bugs in ps_reg

* a few more surface pressure bug fixes; now the test case runs in debug mode

* workarounds and bug fixes from gnu compiler testing

* remove -9999999 commented-out code

* quiet the NaNs passed to Atmp%ps

* simplify comments and explain snan

* use i-1 & j-1 for two-point averages, when available

* Replace many changes with PR NOAA-GFDL#220

Co-authored-by: Rusty.Benson <rusty.benson@noaa.gov>
Co-authored-by: Ted Mansell <37668594+MicroTed@users.noreply.github.com>
Co-authored-by: Rusty Benson <6594772+bensonr@users.noreply.github.com>
Co-authored-by: Samuel Trahan (NOAA contractor) <39415369+SamuelTrahanNOAA@users.noreply.github.com>
Co-authored-by: Mark Potts <33099090+mark-a-potts@users.noreply.github.com>
@bensonr
Copy link
Copy Markdown
Contributor

bensonr commented Mar 15, 2023

@junwang-noaa @kaiyuan-cheng - is this PR still needed?

@bensonr
Copy link
Copy Markdown
Contributor

bensonr commented Feb 28, 2024

@kaiyuan-cheng - it's been just about a year since you were last asked about the relevance of this PR. If it is no longer needed, please respond so I can close it or work to ensure it can be merged.

@kaiyuan-cheng
Copy link
Copy Markdown
Contributor Author

@bensonr I don't think this PR is needed anymore. My proposed change was merged into the emc branch.

@bensonr bensonr closed this Feb 29, 2024
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.

3 participants