Skip to content

Fix convergence stall in IMPTOTAL solver with REF1 and JGS_TERMINATE_DIFFERENCE on unstructured grids#1608

Open
leahcicon wants to merge 1 commit into
NOAA-EMC:developfrom
leahcicon:IMPTOTAL_fix
Open

Fix convergence stall in IMPTOTAL solver with REF1 and JGS_TERMINATE_DIFFERENCE on unstructured grids#1608
leahcicon wants to merge 1 commit into
NOAA-EMC:developfrom
leahcicon:IMPTOTAL_fix

Conversation

@leahcicon

Copy link
Copy Markdown

Pull Request Summary

Fix convergence stall in IMPTOTAL solver with REF1 and JGS_TERMINATE_DIFFERENCE on unstructured grids.

Description

This PR fixes a bug in PDLIB_JACOBI_GAUSS_SEIDEL_BLOCK in w3profsmd_pdlib.F90 where the implicit solver (IMPTOTAL) fails to exit early when using JGS_BLOCK_GAUSS_SEIDEL = T, JGS_TERMINATE_DIFFERENCE = T, and the REF1 switch on unstructured grids. Instead of converging early, the solver stalls at approximately 16% converged nodes and runs to the maximum number of iterations. The number of converged nodes will change depending on the amount of grid nodes affected by REF1.

Root Cause:
The convergence metric was computed as:

DiffNew = abs(sum(ACLOC - eSum)) / Sum_new

Where ACLOC is the previous spectrum and eSum is the newly computed value. However, when REF1 is active, after eSum is assigned to VA, certain bins of VA are overwritten with VAOLD:

#ifdef W3_REF1
  DO IK=1,NK
    DO ITH=1,NTH
      ISP  = ITH + (IK-1)*NTH
      IF (REFPARS(3) .LT. 0.5 .AND. IOBPD_LOC(ITH,IP) .EQ. 0 .AND. IOBPA_LOC(IP) .EQ. 0) THEN
        VA(ISP,IP) = VAOLD(ISP,IP) * IOBDP_LOC(IP)
      ENDIF
    ENDDO
  ENDDO
#endif

On the next iteration, ACLOC picks up these restored values. The result is that the interior nodes converge normally but the REF1 boundary bins never change between iterations. The residual |ACLOC - eSum| remains nonzero for those bins because eSum does not include the REF1 overwrite. prop_conv stalls at approximately the fraction of non-REF1 nodes and the JGS_TERMINATE_DIFFERENCE condition is never satisfied, causing the solver to run to maximum iterations.

Fix:
The convergence metric is changed to compare against VA after the REF1 overwrite has been applied, rather than eSum before it:

IF (B_JGS_TERMINATE_DIFFERENCE) THEN
  Sum_New = sum(VA(:,IP))
  if (Sum_new .gt. 0.d0) then
    DiffNew = abs(sum(ACLOC-VA(:,IP)))/Sum_new
    p_is_converged = DiffNew
  else
    p_is_converged = zero
  endif
  IF (p_is_converged .lt. B_JGS_DIFF_THR .and. nbiter .gt. 1) then
    is_converged   = is_converged + 1
    lconverged(ip) = .true.
  ELSE
    lconverged(ip) = .false.
  ENDIF
END IF

With this fix, for REF1 bins ACLOC - VA = 0, so those bins no longer pollute the convergence metric and the solver exits normally once the interior nodes converge.

Labels: bug
Answer changes: No change in answers is expected, the changes just allow for the early exit to happen.

Issue(s) addressed

Commit Message

Fix convergence stall in IMPTOTAL solver with REF1 and JGS_TERMINATE_DIFFERENCE on unstructured grids. The convergence metric in PDLIB_JACOBI_GAUSS_SEIDEL_BLOCK in w3profsmd_pdlib.F90 now compares against VA after REF1 overwrite is applied rather than eSum before it, preventing REF1 boundary bins from polluting the residual and stalling prop_conv.

Check list

Testing

How were these changes tested? Preliminary runs with the fix applied show the solver exiting early around ~70 iterations as expected, rather than running to the maximum iteration count.
Are the changes covered by regression tests? Not currently. A regression test covering IMPTOTAL with REF1 on an unstructured grid with JGS_TERMINATE_DIFFERENCE = T would be needed to fully cover this fix.
Have the matrix regression tests been run? No
Expected changes in regression test output: No changes expected in existing regression tests since this code path is only active with JGS_TERMINATE_DIFFERENCE = T and REF1 on unstructured grids.
matrix.comp output: Not available.

@mingchen-NOAA

Copy link
Copy Markdown
Collaborator

Matrix Comparison tests were performed on Ursa-Intel.

Known non-identical cases

mww3_test_03/./work_PR3_UNO_MPI_d2                     (14 files differ)
mww3_test_03/./work_PR3_UQ_MPI_e                     (1 files differ)
mww3_test_03/./work_PR3_UNO_MPI_e                     (1 files differ)
mww3_test_03/./work_PR1_MPI_d2                     (14 files differ)
mww3_test_03/./work_PR3_UQ_MPI_e_c                     (1 files differ)
mww3_test_03/./work_PR2_UNO_MPI_d2                     (17 files differ)
mww3_test_03/./work_PR2_UQ_MPI_d2                     (15 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2_c                     (16 files differ)
mww3_test_03/./work_PR2_UNO_MPI_e                     (1 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2                     (16 files differ)
mww3_test_03/./work_PR1_MPI_e                     (1 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2_c                     (15 files differ)
mww3_test_03/./work_PR3_UNO_MPI_e_c                     (1 files differ)
mww3_test_09/./work_MPI_ASCII                     (0 files differ)
ww3_tp2.10/./work_MPI_OMPH                     (7 files differ)
ww3_tp2.6/./work_ST4_ASCII                     (0 files differ)
ww3_ufs1.3/./work_a                     (3 files differ)

Unexpected differs

ww3_tp2.17/./work_ice_restart                     (34 files differ)
ww3_tp2.17/./work_c                     (8 files differ)
ww3_tp2.17/./work_deep                     (44 files differ)
ww3_tp2.17/./work_iceB                     (44 files differ)
ww3_tp2.17/./work_deepB                     (44 files differ)
ww3_tp2.17/./work_mc                     (6 files differ)
ww3_tp2.17/./work_ice                     (44 files differ)
ww3_tp2.17/./work_mc1                     (6 files differ)
ww3_tp2.17/./work_deep_restart                     (34 files differ)
ww3_tp2.19/./work_1A_a                     (9 files differ)
ww3_tp2.19/./work_1C_a                     (9 files differ)
ww3_tp2.19/./work_1B_a                     (9 files differ)
ww3_tp2.21/./work_b                     (6 files differ)
ww3_tp2.21/./work_b_metis                     (6 files differ)
ww3_tp2.21/./work_mb                     (7 files differ)
ww3_tp2.6/./work_pdlib                     (11 files differ)
ww3_ufs1.1/./work_unstr_c                     (365 files differ)
ww3_ufs1.1/./work_unstr_d                     (365 files differ)

matrixCompFull.txt
matrixCompSummary.txt

@mingchen-NOAA

Copy link
Copy Markdown
Collaborator

@leahcicon In your PR description, you mentioned

'No changes expected in existing regression tests since this code path is only active with JGS_TERMINATE_DIFFERENCE = T and REF1 on unstructured grids.'

However, our regression testing shows a number of unexpected result changes. Could you help explain why these changes are occurring and confirm whether they are expected?

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.

2 participants