Remove repetitive code in module_sf_urban.F by epn09 · Pull Request #1593 · wrf-model/WRF

epn09 · 2021-12-07T12:12:55Z

TYPE: enhancement

KEYWORDS: urban physics

SOURCE: Do Ngoc Khanh (Tokyo Institute of Technology)

DESCRIPTION OF CHANGES:
Problem: There was a code block repeated 12 times.

Solution: Purely refactor, no change in algorithm.

LIST OF MODIFIED FILES:
M phys/module_sf_urban.F

TESTS CONDUCTED

Confirmed bit-by-bit indentical output, before vs after.
Jenkins tests are all passing.

RELEASE NOTE: Remove repetitive code in module_sf_urban.F

davegill · 2021-12-07T13:02:10Z

phys/module_sf_urban.F

-           IF( IVGTYP(I,J) == LCZ_1) THEN
-               UTYPE_URB2D(I,J) = 1  ! high-intensity
-               UTYPE_URB = UTYPE_URB2D(I,J)  ! high-intensity
-              IF (HGT_URB2D(I,J)>0.) THEN
-                  CONTINUE
-               ELSE
-                  WRITE(mesg,*) 'USING DEFAULT URBAN MORPHOLOGY'
-                  WRITE_MESSAGE(mesg)
-                  LP_URB2D(I,J)=0.
-                  LB_URB2D(I,J)=0.
-                  HGT_URB2D(I,J)=0.
-                  IF ( sf_urban_physics == 1 ) THEN
-                     MH_URB2D(I,J)=0.
-                     STDH_URB2D(I,J)=0.
-                     DO K=1,4
-                       LF_URB2D(I,K,J)=0.
-                     ENDDO
-                  ELSE IF ( ( sf_urban_physics == 2 ) .or. ( sf_urban_physics == 3 ) ) THEN
-                     DO K=1,num_urban_hi
-                        HI_URB2D(I,K,J)=0.
-                     ENDDO
-                  ENDIF
-               ENDIF
-                IF (FRC_URB2D(I,J)>0.and.FRC_URB2D(I,J)<=1.) THEN
-                  CONTINUE
-                ELSE
-                  WRITE(mesg,*) 'WARNING, FRC_URB2D = 0 BUT IVGTYP IS URBAN'
-                  WRITE_MESSAGE(mesg)
-                  WRITE(mesg,*) 'WARNING, THE URBAN FRACTION WILL BE READ FROM URBPARM.TBL'
-                  WRITE_MESSAGE(mesg)
-                  FRC_URB2D(I,J) = FRC_URB_TBL(UTYPE_URB)
-                ENDIF
-              SWITCH_URB=1
-            ENDIF


Should this repeated block appear one time? Do you remove all occurrences?

@davegill One occurrence is kept in the end. I'll do one test with the urban options, but where should I upload the test result to?

davegill · 2021-12-07T13:02:47Z

@epn09
It looks as if there is missing code that needs to be included at least one time.

dudhia · 2021-12-07T17:12:56Z

@CenLin He ***@***.***> This refers to the LCZ changes where each LCZ has a duplicate code section. Can you check this?

…

On Tue, Dec 7, 2021 at 6:02 AM Dave Gill ***@***.***> wrote: @epn09 <https://github.com/epn09> It looks as if there is missing code that needs to be included at least one time. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1593 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEIZ77BELULC6F6JMETANM3UPYAYFANCNFSM5JRDALHA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

cenlinhe · 2021-12-07T17:34:51Z

No, these parts should not be removed. These if-statements are used to assign the correct values to UTYPE_URB2D and UTYPE_URB according to the LCZ value in each grid. But I agree that there are some repeated codes within each if-statement that could be further refined. However, since this will not affect the model efficiency or accuracy, I suggest not doing any changes to this part right now, or at least the person who submitted this PR should first check with the original code developer @andreazonato

epn09 · 2021-12-07T23:11:12Z

@cenlinhe I understand the purpose of the original code but from LCZ_1 to LCZ_11, the difference is only in the first two lines (written below), the following lines are purely copy and paste.

           IF( IVGTYP(I,J) == LCZ_1) THEN
               UTYPE_URB2D(I,J) = 1  ! high-intensity

epn09 · 2021-12-07T23:14:11Z

phys/module_sf_urban.F

-           IF( IVGTYP(I,J) == LCZ_9) THEN
-               UTYPE_URB2D(I,J) = 9  ! high-intensity
-               UTYPE_URB = UTYPE_URB2D(I,J)  ! high-intensity
+            IF (SWITCH_URB == 1) THEN


@davegill The diff is too large. The "missing code" that you mentioned starts from here.

cenlinhe · 2021-12-08T00:21:27Z

@epn09 Yes, you are right. Could you please do a test to see if your refined code produces exactly the same results as the original code? If so, we can approve the change. Also, another suggestion: it seems that your modifications are too scattered (e.g., insert or remove things in between if-else statement, etc.), which makes it difficult to check in the 'Files changed' part. It may be better to remove all the repeated parts first, and then lump together your refined code in one integrated part so that it would be easier to compare the new code and original code.

epn09 · 2021-12-08T05:02:51Z

@cenlinhe I have split the original commit into 6 smaller commits. Is it easier to understand for you?
I'm also running a test to confirm if the refined version produce the same result.

epn09 · 2021-12-09T09:50:47Z

@cenlinhe @davegill I have conducted a test with urban options and confirm that the output of the original code and the modified code are exactly the same (bit-by-bit equal). How should I share the result?

andreazonato · 2021-12-09T16:03:11Z

Hi all,

I think these modifications make sense, they decrease the computational time for sure.

The only doubt I have regards the first SWITCH_URB=1. Is it in the right position?

Andrea

epn09 · 2021-12-09T23:07:14Z

Hi @andreazonato,

For your concern, you can take a look at this comit a8d55d7 or the explanation below.

Originally, the code was

SWITCH_URB = 0

IF (IVGTYP(I,J) == ISURBAN) THEN
  ! do some thing for ISURBAN
  SWITCH_URB = 1
ENDIF
IF (IVGTYP(I,J) == LCZ_1) THEN
  ! do some thing for LCZ_1
  SWITCH_URB = 1
ENDIF

IF (SWITCH_URB == 0) THEN
  CONTINUE
ELSE
  ! do something for non-urban
ENDIF

The first two ifs are mutually exclusive, so we can combine with ELSE.

SWITCH_URB = 0

IF (IVGTYP(I,J) == ISURBAN) THEN
  ! do some thing for ISURBAN
  SWITCH_URB = 1
ELSE IF (IVGTYP(I,J) == LCZ_1) THEN
  ! do some thing for LCZ_1
  SWITCH_URB = 1
ENDIF

IF (SWITCH_URB == 0) THEN
  CONTINUE
ELSE
  ! do something for non-urban
ENDIF

The role of SWITCH_URB is to check if we've hit any of ISURBAN, LCZ_1, ..., LCZ_11, thus, initially we can set it to 1, and change it back to 0 if none of the ifs match.

SWITCH_URB = 1

IF (IVGTYP(I,J) == ISURBAN) THEN
  ! do some thing for ISURBAN
ELSE IF (IVGTYP(I,J) == LCZ_1) THEN
  ! do some thing for LCZ_1
ELSE
  SWITCH_URB = 0
ENDIF

IF (SWITCH_URB == 0) THEN
  CONTINUE
ELSE
  ! do something for non-urban
ENDIF

davegill · 2021-12-15T02:12:40Z

@andreazonato @cenlinhe
Folks,
Have your concerns been addressed? We would like to move these PRs into the develop branch if they are ready. Let us know if you have other issues, or are able to accept these mods, AS-IS. A simple "I am OK with these mods" is sufficient.

epn09 · 2021-12-28T06:26:17Z

@davegill @andreazonato @cenlinhe
Hi, is there any update on this?

andreazonato · 2021-12-28T10:11:55Z

Yes, in my opinion it makes sense. I agree with the modifications!

cenlinhe · 2021-12-28T17:10:52Z

It looks good to me too.

weiwangncar · 2021-12-28T17:23:53Z

@cenlinhe Can you approve this PR? Click on the 'Files changed' option and see if you can see 'Review changes' in a green box on top right. If not, I will do that.

cenlinhe

This re-structuring of the code looks good and the developer has also confirmed the same results before and after this re-structure.

davegill · 2021-12-28T17:53:09Z

@epn09
Would you please add your affiliation to the pull request commit message?
Go to #1593

As shown below, click on the three dots, then select "Edit".

Once you have added your affiliation, at the bottom right, click on the green "Update comment" button.

cenlinhe · 2021-12-28T18:03:57Z

@epn09 To keep a record, could you please also send us the results of your bit-by-bit comparison before and after the change? Any summary tables, figures, and/or small-size data files will be good. My email: cenlinhe@ucar.edu

weiwangncar

The LSM/urban experts have said ok to this code change.

epn09 · 2022-01-02T10:52:07Z

@davegill I have added my affiliation.

epn09 · 2022-01-04T23:52:01Z

@davegill I have sent a copy of my test output to @cenlinhe and he said that it's acceptable.

TYPE: enhancement KEYWORDS: urban physics SOURCE: Do Ngoc Khanh (Tokyo Institute of Technology) DESCRIPTION OF CHANGES: Problem: There was a code block repeated 12 times. Solution: Purely refactor, no change in algorithm. LIST OF MODIFIED FILES: M phys/module_sf_urban.F TESTS CONDUCTED 1. Confirmed bit-by-bit identical output, before vs after. 2. Jenkins tests are all passing. RELEASE NOTE: Remove repetitive code in module_sf_urban.F

epn09 requested a review from a team as a code owner December 7, 2021 12:12

davegill reviewed Dec 7, 2021

View reviewed changes

davegill changed the base branch from master to develop December 7, 2021 13:03

davegill added PHYSICS Enhancement Develop Branch labels Dec 7, 2021

epn09 commented Dec 7, 2021

View reviewed changes

epn09 added 6 commits December 8, 2021 13:55

Remove LCZ_2 to LCZ_11. Will be added back

2c261e2

Take out different part from ISURBAN and LCZ_1

7d29d29

Combine repeated code in ISURBAN and LCZ_1

276f61f

Combine IF (SWITCH_URB == 1)

6165272

Modify SWITCH_URB logic equivalently

a8d55d7

Add LCZ_2 to LCZ_11 back

3ac1417

epn09 force-pushed the refactor_sf_urban branch from 7a714dd to 3ac1417 Compare December 8, 2021 04:57

epn09 requested a review from davegill December 9, 2021 09:52

cenlinhe approved these changes Dec 28, 2021

View reviewed changes

davegill requested a review from weiwangncar December 28, 2021 17:49

weiwangncar approved these changes Dec 28, 2021

View reviewed changes

davegill approved these changes Jan 5, 2022

View reviewed changes

davegill merged commit c518c43 into wrf-model:develop Jan 5, 2022

epn09 deleted the refactor_sf_urban branch January 5, 2022 01:10

Conversation

epn09 commented Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davegill Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

epn09 Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

davegill commented Dec 7, 2021

Uh oh!

dudhia commented Dec 7, 2021 via email

Uh oh!

cenlinhe commented Dec 7, 2021

Uh oh!

epn09 commented Dec 7, 2021

Uh oh!

epn09 Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

cenlinhe commented Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epn09 commented Dec 8, 2021

Uh oh!

epn09 commented Dec 9, 2021

Uh oh!

andreazonato commented Dec 9, 2021

Uh oh!

epn09 commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davegill commented Dec 15, 2021

Uh oh!

epn09 commented Dec 28, 2021

Uh oh!

andreazonato commented Dec 28, 2021

Uh oh!

cenlinhe commented Dec 28, 2021

Uh oh!

weiwangncar commented Dec 28, 2021

Uh oh!

cenlinhe left a comment

Choose a reason for hiding this comment

Uh oh!

davegill commented Dec 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cenlinhe commented Dec 28, 2021

Uh oh!

weiwangncar left a comment

Choose a reason for hiding this comment

Uh oh!

epn09 commented Jan 2, 2022

Uh oh!

epn09 commented Jan 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

epn09 commented Dec 7, 2021 •

edited

Loading

cenlinhe commented Dec 8, 2021 •

edited

Loading

epn09 commented Dec 9, 2021 •

edited

Loading

davegill commented Dec 28, 2021 •

edited

Loading