Skip to content

Syntax errors in physics routines: *- and *+#1577

Merged
davegill merged 1 commit intowrf-model:release-v4.3.2from
davegill:hpe
Nov 29, 2021
Merged

Syntax errors in physics routines: *- and *+#1577
davegill merged 1 commit intowrf-model:release-v4.3.2from
davegill:hpe

Conversation

@davegill
Copy link
Contributor

@davegill davegill commented Nov 5, 2021

TYPE: bug fix

KEYWORDS: HPE, CCE, syntax

SOURCE: Tricia Balle (HPE)

DESCRIPTION OF CHANGES:
Problem:
There were three types of syntax errors that were uncovered by the HPE CCE compiler.

  1. Open statement used action="append".
  2. Multiple unary operators next to each other *+
  3. Multiple unary operators next to each other *-1.

Solution:

  1. The correct usage is position="append".
  2. The developer agrees with the new modification to GS3*+SASR1 was the original intent.
  3. Typically, *(-1) is intended, and this is the case.

Via some grep'ing, no other occurrences of *+ or *- exist in the physics directory in the compilable
source code. Syntactically OK source code with /- (such as for DATA statements) were the only compilable
examples found. So, we fixed the above problems identified in this PR, and we could not find any other
similar examples in the source code.

LIST OF MODIFIED FILES:
modified: phys/module_mp_nssl_2mom.F
modified: phys/module_mp_ntu.F
modified: phys/module_sf_noahmplsm.F

TESTS CONDUCTED:

  1. These fixes allow the WRF code to build with the CCE compiler.
  2. Jenkins tests are all PASS.
  3. One of the errors was a typo in the sink term of aggregates number from hail. A quick evaluation of its impact was
    quite minor from the 2D idealized simulation (the figure is not shown).

RELEASE NOTE: A few syntax errors in NSSL, NTU, an NoahMP were fixed. Most compilers skipped over them. No impact for users for whom the code already compiled.

TYPE: bug fix

KEYWORDS: HPE, CCE, syntax

SOURCE: Tricia Balle (HPE)

DESCRIPTION OF CHANGES:
Problem:
There were three types of syntax errors that were uncovered by the HPE CCE compiler.
1. Open statement used `action="append"`.
2. Multiple unary operators next to each other `*+`
3. Multiple unary operators next to each other `*-`.

Solution:
1. The correct usage is `position="append"`.
2. Not sure what is intended. Need to get the developer to weigh in on `GS3*+SASR1`.
3. Typically, `*(-1)` is intended, and this looks to be the case.

LIST OF MODIFIED FILES:
modified:   phys/module_mp_nssl_2mom.F
modified:   phys/module_mp_ntu.F
modified:   phys/module_sf_noahmplsm.F

TESTS CONDUCTED:
1. Fixes all the WRF code to build with the CCE compiler.
2. Hoping that all Jenkins tests are a PASS.

RELEASE NOTE: A few syntax errors in NSSL, NTU, an NoahMP were fixed. Most compilers skipped over them. No impact for users for whom the code already compiled.
@davegill davegill requested a review from a team as a code owner November 5, 2021 19:17
@davegill
Copy link
Contributor Author

davegill commented Nov 5, 2021

@weiwangncar @dudhia
Folks,
The modification to the NTU scheme needs to be reviewed. I have no idea what was intended. This probably needs to get back to the developer.

QCLsh = QCLS1*ESH*VTQSH*NH1D*(SASR2*GSM3+SASR1*2.*GH2*GSM2+&
GH3*GSM1)
NCLsh = NCLS1*ESH*VTNSH*NH1D*(SASR2*GS3*+SASR1*2.*GH2*GS2+ &
NCLsh = NCLS1*ESH*VTNSH*NH1D*(SASR2*GS3 +SASR1*2.*GH2*GS2+ &
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weiwangncar @dudhia
This one is the biggy

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davegill This looks good to me. I'll ask the author to approve it too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Wei.
Could we also ask the author to comment on the likely impact of multiplying, instead of adding those terms? That might be significant.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davegill I sent him this page, and hopefully he will see your comment.

@davegill
Copy link
Contributor Author

davegill commented Nov 5, 2021

jenkins is OK

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

    Test Type              | Expected  | Received |  Failed
    = = = = = = = = = = = = = = = = = = = = = = = =  = = = =
    Number of Tests        : 19           17
    Number of Builds       : 48           45
    Number of Simulations  : 160           155        0
    Number of Comparisons  : 101           100        0

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

Copy link
Contributor

@tzuchin12 tzuchin12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks. It is a typo in the sink term of aggregates number from hail. The quick evaluation of its impact is quite minor from the 2D idealized simulation (the figure is not shown).

@davegill davegill changed the title Small physics syntax errors Syntax errors in physics routines: *- and *+ Nov 7, 2021
@milancurcic
Copy link
Contributor

Open statement used action="append".

FWIW, we just hit this with the IBM compiler (xlf).

Errors like this can be caught by CI, for example by disabling non-standard extensions with gfortran:

$ gfortran -std=f2018 test.f90 
test.f90:1:41:

    1 | open(20, file='new.txt', access='append')
      |                                         1
Error: GNU Extension: ACCESS specifier in OPEN statement at (1) has value ‘APPEND’

I tried looking for a similar option with the Intel compiler and couldn't find it. At least ifort -stand f18 -standard-semantics -warn all doesn't catch this.

@davegill davegill merged commit 7c6fd57 into wrf-model:release-v4.3.2 Nov 29, 2021
davegill added a commit to davegill/WRF that referenced this pull request Dec 9, 2021
TYPE: bug fix

KEYWORDS: syntax, physics *+

SOURCE: Patricia Balle (HPE)

DESCRIPTION OF CHANGES:
Problem:
As in hash 7c6fd57, PR wrf-model#1577 "Syntax errors in physics routines: *- and *+", there is a single
remaining example of the unary operators next to each other.

Solution:
The author agreed that the `*+` combination was suppoed to be just `+`.

LIST OF MODIFIED FILES:
modified:   phys/module_mp_ntu.F

TESTS CONDUCTED:
1. Passes Cray compiler
2. Jenkins tests are OK
davegill added a commit that referenced this pull request Dec 9, 2021
TYPE: bug fix

KEYWORDS: syntax, physics, *+

SOURCE: Patricia Balle (HPE)

DESCRIPTION OF CHANGES:
Problem:
As in hash 7c6fd57, PR #1577 "Syntax errors in physics routines: *- and *+", there is a single
remaining example of the unary operators next to each other.

Solution:
The author agreed that the `*+` combination was supposed to be just `+`.

I searched the WRF code for instances of this `*+` in PR #1577. When I did not find any, I put back one of them from 
NTU code to make sure that my grep was working. It found that example, and no others that I cared about. I forgot
to undo that change before the `git add`, `git commit`.

LIST OF MODIFIED FILES:
modified:   phys/module_mp_ntu.F

TESTS CONDUCTED:
1. Passes Cray compiler.
2. Jenkins tests are OK
vlakshmanan-scala pushed a commit to scala-computing/WRF that referenced this pull request Apr 4, 2024
TYPE: bug fix

KEYWORDS: HPE, CCE, syntax

SOURCE: Tricia Balle (HPE)

DESCRIPTION OF CHANGES:
Problem:
There were three types of syntax errors that were uncovered by the HPE CCE compiler.
1. Open statement used `action="append"`.
2. Multiple unary operators next to each other `*+`
3. Multiple unary operators next to each other `*-1`.

Solution:
1. The correct usage is `position="append"`.
2. The developer agrees with the new modification to `GS3*+SASR1` was the original intent.
3. Typically, `*(-1)` is intended, and this is the case.

Via some grep'ing, no other occurrences of `*+` or `*-` exist in the physics directory in the compilable
source code. Syntactically OK source code with `/-` (such as for DATA statements) were the only compilable 
examples found. So, we fixed the above problems identified in this PR, and we could not find any other 
similar examples in the source code.

LIST OF MODIFIED FILES:
modified:   phys/module_mp_nssl_2mom.F
modified:   phys/module_mp_ntu.F
modified:   phys/module_sf_noahmplsm.F

TESTS CONDUCTED:
1. These fixes allow the WRF code to build with the CCE compiler.
2. Jenkins tests are all PASS.
3. One of the errors was a typo in the sink term of aggregates number from hail. A quick evaluation of its impact was
quite minor from the 2D idealized simulation (the figure is not shown).

RELEASE NOTE: A few syntax errors in NSSL, NTU, an NoahMP were fixed. Most compilers skipped over them. Either "small impact" or "no impact" for users for whom the code already compiled.
vlakshmanan-scala pushed a commit to scala-computing/WRF that referenced this pull request Apr 4, 2024
)

TYPE: bug fix

KEYWORDS: syntax, physics, *+

SOURCE: Patricia Balle (HPE)

DESCRIPTION OF CHANGES:
Problem:
As in hash 7c6fd57, PR wrf-model#1577 "Syntax errors in physics routines: *- and *+", there is a single
remaining example of the unary operators next to each other.

Solution:
The author agreed that the `*+` combination was supposed to be just `+`.

I searched the WRF code for instances of this `*+` in PR wrf-model#1577. When I did not find any, I put back one of them from 
NTU code to make sure that my grep was working. It found that example, and no others that I cared about. I forgot
to undo that change before the `git add`, `git commit`.

LIST OF MODIFIED FILES:
modified:   phys/module_mp_ntu.F

TESTS CONDUCTED:
1. Passes Cray compiler.
2. Jenkins tests are OK
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants