Fix occurrence of time for max/min of diagnostics in clWRF by christopherthomas · Pull Request #1208 · wrf-model/WRF

christopherthomas · 2020-06-11T02:23:00Z

TYPE: bug fix

KEYWORDS: clWRF, maximum, minimum, time, diagnostics

SOURCE: Christopher Thomas (Climate Change Research Centre, UNSW Australia)

DESCRIPTION OF CHANGES:
TT2MAX and TT2MIN are the times at which T2MAX and T2MIN, the max and min values of
T2 within a diagnostic period, are achieved. This bug caused the incorrect computation of
TT2MAX and TT2MIN for cases in which T2MAX or T2MIN occurred at the beginning of the diagnostic period under consideration. At the beginning of each diagnostic period the working
values of T2MAX and T2MIN are reinitialized with the current value of T2, however the working
values of TT2MAX and TT2MIN were not reinitialized with the current time value. Thus if the
max or min actually occurred at the beginning of the diagnostic period, then TT2MAX or
TT2MIN would contain the value assigned to it in the previous period. The bug fix corrects
this by re-initializing TT2MAX and TT2MIN at the beginning of each diagnostic period.

The same initialization of the time of occurrence for the max/min of the 2-m temperature was
applied to these variables: tt2clmin, tt2clmax, tq2clmin, tq2clmax, tspduv10clmax,
traincclmax, trainncclmax, tskintempclmin, tskintempclmax.

ISSUE:
Closes #1193

LIST OF MODIFIED FILES:
M phys/module_diag_cl.F

TESTS CONDUCTED:

Tested on the WRF simulation set up for which the problem was first encountered. The fix
changes the TT2MAX and TT2MIN values only for those points/periods for which they were
incorrect, leaving all others unchanged. The values of TT2MAX and TT2MIN produced by the
fix are in agreement with those computed by outputting T2 for all times steps and calculating
TT2MAX and TT2MIN directly (using R). The tests were made using v4.1.2 but the file in
question (module_diag_cl.F) has not changed in v4.2.
Here the diagnostic period for the min/max output is 24 hrs. Screenshot shows second
24 hr period of simulation.
Left-hand panel = before fix
Right-hand panel = after fix
Values for TT2MAX should range between 1440 and 2880
Note speckling in region south of Australia, also isolated instances elsewhere. In this instance
nothing seems to occur over land but it clearly could. Bug only manifests if the extreme value
occurs at the beginning of the diagnostic period.
Jenkins testing is OK.

RELEASE NOTE: The calculation of the times of minimum and maximum of fields within the clWRF diagnostics module was occasionally incorrect. At the beginning of each diagnostic period the working values of diagnostic was correctly reinitialized with the current value; however the working values of occurrence of the time of the max/min values of the diagnostics were not reinitialized with the current time value. Thus if the max or min actually occurred at the beginning of the diagnostic period, then the reported time would incorrectly contain the value assigned during the previous output period.

davegill · 2020-06-11T14:10:15Z

jenkins is OK

Please find result of the WRF regression test cases in the attachment. This build is for Commit ID: 86567d64795e442f26fa9c713c9df7719cb35adb, requested by: christopherthomas for PR: https://github.com/wrf-model/WRF/pull/1208. For any query please send e-mail to David Gill.

    Test Type              | Expected  | Received |  Failed
    = = = = = = = = = = = = = = = = = = = = = = = =  = = = =
    Number of Tests        : 19           18
    Number of Builds       : 48           46
    Number of Simulations  : 166           164        0
    Number of Comparisons  : 105           104        0

    Failed Simulations are: 
    None
    Which comparisons are not bit-for-bit: 
    None

dudhia · 2020-06-11T15:34:19Z

I put a comment on the Issues thread that there other times that probably also need initializing like this.

…

On Thu, Jun 11, 2020 at 8:10 AM Dave Gill ***@***.***> wrote: jenkins is OK Please find result of the WRF regression test cases in the attachment. This build is for Commit ID: 86567d6, requested by: christopherthomas for PR: #1208. For any query please send e-mail to David Gill. Test Type | Expected | Received | Failed = = = = = = = = = = = = = = = = = = = = = = = = = = = = Number of Tests : 19 18 Number of Builds : 48 46 Number of Simulations : 166 164 0 Number of Comparisons : 105 104 0 Failed Simulations are: None Which comparisons are not bit-for-bit: None — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1208 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEIZ77E7MMU3A6AZBYOHEYTRWDQVNANCNFSM4N27T2PA> .

davegill · 2020-06-11T15:44:24Z

@dudhia @christopherthomas
Jimy,
Good point about other variables.

Christopher,
Here are the other variables in that list:

      DO j = j_start(ij), j_end(ij)
        DO i = i_start(ij), i_end(ij)
          t2clmin(i,j)=t2(i,j)-t273
          t2clmax(i,j)=t2(i,j)-t273
          t2clmean(i,j)=t2(i,j)-t273
          t2clstd(i,j)=(t2(i,j)-t273)*(t2(i,j)-t273)
          q2clmin(i,j)=q2(i,j)
          q2clmax(i,j)=q2(i,j)
          q2clmean(i,j)=q2(i,j)
          q2clstd(i,j)=q2(i,j)*q2(i,j)
          spduv10clmax(i,j)=sqrt(u10(i,j)*u10(i,j)+v10(i,j)*v10(i,j))
          u10clmean(i,j)=u10(i,j)
          v10clmean(i,j)=v10(i,j)
          spduv10clmean(i,j)=sqrt(u10(i,j)*u10(i,j)+v10(i,j)*v10(i,j))
          u10clstd(i,j)=u10(i,j)*u10(i,j)
          v10clstd(i,j)=v10(i,j)*v10(i,j)
          spduv10clstd(i,j)=u10(i,j)*u10(i,j)+v10(i,j)*v10(i,j)
          raincclmax(i,j)=raincv(i,j)/dt
          rainncclmax(i,j)=rainncv(i,j)/dt
          raincclmean(i,j)=raincv(i,j)/dt
          rainncclmean(i,j)=rainncv(i,j)/dt
          raincclstd(i,j)=(raincv(i,j)/dt)*(raincv(i,j)/dt)
          rainncclstd(i,j)=(rainncv(i,j)/dt)*(rainncv(i,j)/dt)
          skintempclmin(i,j)=skintemp(i,j)-t273
          skintempclmax(i,j)=skintemp(i,j)-t273
          skintempclmean(i,j)=skintemp(i,j)-t273
          skintempclstd(i,j)=(skintemp(i,j)-t273)*(skintemp(i,j)-t273)
          tt2clmin(i,j) = xtime + dt/60.  ! value at end of timestep 
          tt2clmax(i,j) = xtime + dt/60.
        ENDDO
      ENDDO

Many of these have the associated "t" (time) variable that ALSO needs to be initialized at the beginning of each collection time.

dudhia · 2020-06-11T15:56:52Z

I only mean the t variables like tq2clmax. There's a few others.

…

On Thu, Jun 11, 2020 at 9:44 AM Dave Gill ***@***.***> wrote: @dudhia <https://github.com/dudhia> @christopherthomas <https://github.com/christopherthomas> Jimy, Good point about other variables. Christopher, Here are the other variables in that list: DO j = j_start(ij), j_end(ij) DO i = i_start(ij), i_end(ij) t2clmin(i,j)=t2(i,j)-t273 t2clmax(i,j)=t2(i,j)-t273 t2clmean(i,j)=t2(i,j)-t273 t2clstd(i,j)=(t2(i,j)-t273)*(t2(i,j)-t273) q2clmin(i,j)=q2(i,j) q2clmax(i,j)=q2(i,j) q2clmean(i,j)=q2(i,j) q2clstd(i,j)=q2(i,j)*q2(i,j) spduv10clmax(i,j)=sqrt(u10(i,j)*u10(i,j)+v10(i,j)*v10(i,j)) u10clmean(i,j)=u10(i,j) v10clmean(i,j)=v10(i,j) spduv10clmean(i,j)=sqrt(u10(i,j)*u10(i,j)+v10(i,j)*v10(i,j)) u10clstd(i,j)=u10(i,j)*u10(i,j) v10clstd(i,j)=v10(i,j)*v10(i,j) spduv10clstd(i,j)=u10(i,j)*u10(i,j)+v10(i,j)*v10(i,j) raincclmax(i,j)=raincv(i,j)/dt rainncclmax(i,j)=rainncv(i,j)/dt raincclmean(i,j)=raincv(i,j)/dt rainncclmean(i,j)=rainncv(i,j)/dt raincclstd(i,j)=(raincv(i,j)/dt)*(raincv(i,j)/dt) rainncclstd(i,j)=(rainncv(i,j)/dt)*(rainncv(i,j)/dt) skintempclmin(i,j)=skintemp(i,j)-t273 skintempclmax(i,j)=skintemp(i,j)-t273 skintempclmean(i,j)=skintemp(i,j)-t273 skintempclstd(i,j)=(skintemp(i,j)-t273)*(skintemp(i,j)-t273) tt2clmin(i,j) = xtime + dt/60. ! value at end of timestep tt2clmax(i,j) = xtime + dt/60. ENDDO ENDDO Many of these have the associated "t" (time) variable that ALSO needs to be initialized at the beginning of each collection time. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1208 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEIZ77EOEA4ICC5KYFZTQM3RWD3WTANCNFSM4N27T2PA> .

christopherthomas · 2020-06-11T21:33:52Z

@davegill
@dudhia
Okay, I didn't realise that. I'll take a look. Thanks.

christopherthomas · 2020-06-12T00:01:40Z

@davegill
@dudhia
I have sorted this out; should have noticed it myself. I haven't pushed this back yet, but:

The logic regarding the times is identical in all the subroutines, and I have done thorough testing for TT2MAX and TT2MIN, do you think that is sufficient for the other variables as well?
Should I edit this pull request or create a new one?

davegill · 2020-06-12T14:12:28Z

@christopherthomas @dudhia
Christopher,

Modify this PR.
Do we have your name and affiliation correctly listed in the PR commit message?

christopherthomas · 2020-06-17T09:37:16Z

TYPE: bug fix

KEYWORDS: TT2MIN, TT2MAX, TQ2MIN, TQ2MAX, TSKINTEMPMIN, TSKINTEMPMAX, TRAINCVMAX, TRAINNCVMAX, TSPDUV10MAX

SOURCE: Christopher Thomas (Climate Change Research Centre, UNSW Australia)

DESCRIPTION OF CHANGES:
TT2MIN, TT2MAX, TQ2MIN, TQ2MAX, TSKINTEMPMIN, TSKINTEMPMAX, TRAINCVMAX, TRAINNCVMAX and TSPDUV10MAX are the times at which the indicated variables (T2, Q2, SKINTEMP, RAINCV, etc) achieve their extreme values within a diagnostic period (maximum, and in some cases minimum). This bug caused the incorrect computation of these times
for cases in which the extreme values occurred at the beginning of the diagnostic period under consideration. At the beginning of each diagnostic period the working
values of TT2MAX, TT2MIN, etc were not reinitialized with the current time value. Thus if the max or min actually occurred at the beginning of the diagnostic period, then the variable containing the time of the extreme value would contain the value assigned to it in the previous period. The bug fix corrects this by re-initializing TT2MIN, TT2MAX, TQ2MIN, TQ2MAX, TSKINTEMPMIN, TSKINTEMPMAX, TRAINCVMAX, TRAINNCVMAX and TSPDUV10MAX with the current time at the beginning of each diagnostic period.

ISSUE:
Closes #1193

LIST OF MODIFIED FILES:
M phys/module_diag_cl.F

TESTS CONDUCTED:
Tested for TT2MIN and TT2MAX on the WRF simulation set up for which the problem was first encountered. The fix changes the TT2MAX and TT2MIN values only for those points/periods for which they were incorrect, leaving all others unchanged. The values of TT2MAX and TT2MIN produced by the fix are in agreement with those computed by outputting T2 for all times steps and calculating TT2MAX and TT2MIN directly (using R). The tests were made using v4.1.2 but the file in question (module_diag_cl.F) has not changed in v4.2. The logic for the other variables (TQ2MIN, TQ2MAX, TSKINTEMPMIN, TSKINTEMPMAX, TRAINCVMAX, TRAINNCVMAX, TSPDUV10MAX) is identical.
Jenkins testing is OK.

RELEASE NOTE: The calculation of TT2MIN, TT2MAX, TQ2MIN, TQ2MAX, TSKINTEMPMIN, TSKINTEMPMAX, TRAINCVMAX, TRAINNCVMAX and TSPDUV10MAX (times of extreme values of T2, Q2, SKINTEMP, RAINCV, RAINNCV and SPDUV10) within the clWRF diagnostics module was occasionally incorrect. At the beginning of each diagnostic period the working values of the extreme value variables (T2MIN, T2MAX, etc) are reinitialized with the current value of of the field (e.g. T2), however the working values of the time variables (TT2MAX, TT2MIN, etc) were not reinitialized with the current time value. Thus if the max or min actually occurred at the beginning of the diagnostic period, then these time variables would contain the value assigned to them in the previous period.

christopherthomas · 2020-06-17T09:40:29Z

@davegill
Yes, my name and affiliation are correct.
Apologies for not noticing the other variables requiring fixes.

Changes to be committed: modified: phys/module_diag_cl.F

davegill · 2020-06-17T13:51:46Z

@dudhia @weiwangncar @kkeene44 @smileMchen
Folks,
This PR seems ready for a review.

It is a bug-fix PR that is for v4.2.1.
The git log is cleanly from the v4.2.1 branch.
The testing with an external program validates that the results are now correct.
Jimy's recommendation to handle all affected diagnostics seems to be completed.
Jenkins is OK
The PR commit message is complete, accurate, and understandable.

dudhia

Looks fine to me.

weiwangncar · 2020-06-17T17:14:39Z

@christopherthomas Do you happen to have a figure that shows the wrong output and one that shows the correct result?

christopherthomas · 2020-06-17T22:39:01Z

@weiwangncar
@davegill
@dudhia
Screenshot attached. Here the diagnostic period for the min/max output is 24 hrs. Screenshot shows second 24 hr period of simulation.
Left-hand panel = before fix
Right-hand panel = after fix
Values for TT2MAX should range between 1440 and 2880
Note speckling in region south of Australia, also isolated instances elsewhere. In this instance nothing seems to occur over land but it clearly could. Bug only manifests if the extreme value occurs at the beginning of the diagnostic period.

davegill · 2020-06-17T22:48:16Z

@christopherthomas

Note speckling in region south of Australia

I have included your figs in the PR commit message. The figs present a clear picture of what is happening. Thanks for providing them.

davegill · 2020-06-17T22:49:45Z

@weiwangncar
Wei,
I am good with this, too. Let me know if you are OK.

weiwangncar · 2020-06-17T23:55:37Z

@christopherthomas Thanks for providing the plots.
@davegill I'm now ok with this PR. Note that he updated his PR message.

davegill · 2020-06-17T23:59:14Z

@weiwangncar

Note that he updated his PR message.

Wei,
I cleaned that up.

davegill

Wei and Jimy say OK

Bug fix to module_diag_cl.F to correct computation of TT2MIN and TT2MAX

48afddf

christopherthomas requested a review from a team as a code owner June 11, 2020 02:23

davegill added 2 commits June 11, 2020 00:24

Maybe fix indenting?

13f2215

Maybe this time?

86567d6

davegill added bug release-v4.2.1 labels Jun 11, 2020

christopherthomas added 2 commits June 17, 2020 10:15

Added initialization for the other time variables

80bf4f2

Merged Dave Gill's changes to indentation

3d353f2

christopherthomas marked this pull request as draft June 17, 2020 08:45

Clean up tabs vs spaces

122ac82

Changes to be committed: modified: phys/module_diag_cl.F

davegill changed the title ~~Bug fix for computation of TT2MIN and TT2MAX in clWRF diagnostics~~ Fix occurrence of time for max/min of diagnostics in clWRF Jun 17, 2020

davegill marked this pull request as ready for review June 17, 2020 13:46

dudhia reviewed Jun 17, 2020

View reviewed changes

davegill self-requested a review June 17, 2020 23:59

davegill approved these changes Jun 18, 2020

View reviewed changes

kkeene44 merged commit 74746a4 into wrf-model:release-v4.2.1 Jul 10, 2020

Conversation

christopherthomas commented Jun 11, 2020 • edited by davegill Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davegill commented Jun 11, 2020

Uh oh!

dudhia commented Jun 11, 2020 via email

Uh oh!

davegill commented Jun 11, 2020

Uh oh!

dudhia commented Jun 11, 2020 via email

Uh oh!

christopherthomas commented Jun 11, 2020

Uh oh!

christopherthomas commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davegill commented Jun 12, 2020

Uh oh!

christopherthomas commented Jun 17, 2020

Uh oh!

christopherthomas commented Jun 17, 2020

Uh oh!

davegill commented Jun 17, 2020

Uh oh!

dudhia left a comment

Choose a reason for hiding this comment

Uh oh!

weiwangncar commented Jun 17, 2020

Uh oh!

christopherthomas commented Jun 17, 2020

Uh oh!

davegill commented Jun 17, 2020

Uh oh!

davegill commented Jun 17, 2020

Uh oh!

weiwangncar commented Jun 17, 2020

Uh oh!

davegill commented Jun 17, 2020

Uh oh!

davegill left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

christopherthomas commented Jun 11, 2020 •

edited by davegill

Loading

christopherthomas commented Jun 12, 2020 •

edited

Loading