Skip to content

Update in-line post for RRFS and GFS#641

Merged
jkbk2004 merged 13 commits into
NOAA-EMC:developfrom
WenMeng-NOAA:upp_apr
Apr 14, 2023
Merged

Update in-line post for RRFS and GFS#641
jkbk2004 merged 13 commits into
NOAA-EMC:developfrom
WenMeng-NOAA:upp_apr

Conversation

@WenMeng-NOAA
Copy link
Copy Markdown
Contributor

@WenMeng-NOAA WenMeng-NOAA commented Mar 30, 2023

Description

  • Update the upp revision with the latest commit at UPP repository
  • Update the in-line post read interface for RRFS, GEFS-Aerosols

Issue(s) addressed

Link the issues to be closed with this PR, whether in this repository, or in another repository.
(Remember, issues should always be created before starting work on a PR branch!)

  • fixes #<issue_number>
  • fixes ufs-weather-model/issues/1688

Testing

How were these changes tested?
What compilers / HPCs was it tested with?
Are the changes covered by regression tests? (If not, why? Do new tests need to be added?)
Have the ufs-weather-model regression test been run? On what platform?

  • Will the code updates change regression test baseline? If yes, why? Please show the baseline directory below.
  • Please commit the regression test log files in your ufs-weather-model branch

Dependencies

If testing this branch requires non-default branches in other repositories, list them.
Those branches should have matching names (ideally)

Do PRs in upstream repositories need to be merged first?
If so add the "waiting for other repos" label and list the upstream PRs

  • waiting on noaa-emc/nems/pull/<pr_number>
  • waiting on noaa-emc/fv3atm/pull/<pr_number>

@WenMeng-NOAA WenMeng-NOAA changed the title Update in-line post for RRFS Update in-line post for RRFS and GFS Apr 5, 2023
Comment thread io/post_fv3.F90 Outdated
Comment thread io/post_fv3.F90 Outdated
Comment thread io/post_fv3.F90 Outdated
Comment thread io/post_fv3.F90 Outdated
@WenMeng-NOAA
Copy link
Copy Markdown
Contributor Author

@DusanJovic-NOAA I updated io/post_fv3.F90 based on your comment.

@WenMeng-NOAA
Copy link
Copy Markdown
Contributor Author

@junwang-noaa and @DusanJovic-NOAA Any actions are needed from my side? Thanks!

Comment thread io/post_fv3.F90
Comment thread io/post_fv3.F90
Comment thread io/post_fv3.F90 Outdated
Comment thread io/post_fv3.F90 Outdated
@WenMeng-NOAA
Copy link
Copy Markdown
Contributor Author

@DusanJovic-NOAA Please let me know your comment on the commit 78cfb35.

Comment thread io/post_fv3.F90 Outdated
Comment thread io/post_fv3.F90 Outdated
Comment thread io/post_fv3.F90 Outdated
@DusanJovic-NOAA
Copy link
Copy Markdown
Collaborator

Not related to this PR, but since I'm looking at the code I thought I should mention, there are many OMP directives in which im is listed as shared variable but it is not used in the code at all, for example

!$omp parallel do default(none) private(i,j) shared(jsta,jend,im,spval,pt,pd,pint,ista,iend)
      do j=jsta,jend
        do i=ista,iend
          pd(i,j)     = spval
          pint(i,j,1) = pt
        end do
      end do

Also, code indentation is terrible, it is very difficult to reason about the code and understand the logic with inconsistent indentation. This file (post_fv3.F90) requires some serious clean up.

@WenMeng-NOAA
Copy link
Copy Markdown
Contributor Author

Not related to this PR, but since I'm looking at the code I thought I should mention, there are many OMP directives in which im is listed as shared variable but it is not used in the code at all, for example

!$omp parallel do default(none) private(i,j) shared(jsta,jend,im,spval,pt,pd,pint,ista,iend)
      do j=jsta,jend
        do i=ista,iend
          pd(i,j)     = spval
          pint(i,j,1) = pt
        end do
      end do

Also, code indentation is terrible, it is very difficult to reason about the code and understand the logic with inconsistent indentation. This file (post_fv3.F90) requires some serious clean up.

@DusanJovic-NOAA We could submit another PR to clean up these non-fatal bugs/typos later.

@DusanJovic-NOAA
Copy link
Copy Markdown
Collaborator

Not related to this PR, but since I'm looking at the code I thought I should mention, there are many OMP directives in which im is listed as shared variable but it is not used in the code at all, for example

!$omp parallel do default(none) private(i,j) shared(jsta,jend,im,spval,pt,pd,pint,ista,iend)
      do j=jsta,jend
        do i=ista,iend
          pd(i,j)     = spval
          pint(i,j,1) = pt
        end do
      end do

Also, code indentation is terrible, it is very difficult to reason about the code and understand the logic with inconsistent indentation. This file (post_fv3.F90) requires some serious clean up.

@DusanJovic-NOAA We could submit another PR to clean up these non-fatal bugs/typos later.

Sure. Like I said, not related to this PR.

@WenMeng-NOAA
Copy link
Copy Markdown
Contributor Author

@DusanJovic-NOAA @junwang-noaa @zhanglikate @EricJames-NOAA Please let me know your comments on my latest commit 246acf2.

@jkbk2004
Copy link
Copy Markdown
Collaborator

tests are all done at weather model pr: ufs-community/ufs-weather-model#1690 @DusanJovic-NOAA @zhanglikate @EricJames-NOAA Please, go ahead for final reviews and approvals. @WenMeng-NOAA do you think we need to create an issue to follow up Dusan's comments to clean up codes?

@WenMeng-NOAA
Copy link
Copy Markdown
Contributor Author

tests are all done at weather model pr: ufs-community/ufs-weather-model#1690 @DusanJovic-NOAA @zhanglikate @EricJames-NOAA Please, go ahead for final reviews and approvals. @WenMeng-NOAA do you think we need to create an issue to follow up Dusan's comments to clean up codes?

@jkbk2004 Yes, we can create an issue for that at fv3atm repos. Do you want me to create or you will do it? Thanks!

@jkbk2004
Copy link
Copy Markdown
Collaborator

@WenMeng-NOAA we can resolve the conversations and merge in this pr. can you resolve?

@jkbk2004
Copy link
Copy Markdown
Collaborator

tests are all done at weather model pr: ufs-community/ufs-weather-model#1690 @DusanJovic-NOAA @zhanglikate @EricJames-NOAA Please, go ahead for final reviews and approvals. @WenMeng-NOAA do you think we need to create an issue to follow up Dusan's comments to clean up codes?

@jkbk2004 Yes, we can create an issue for that at fv3atm repos. Do you want me to create or you will do it? Thanks!

@jkbk2004 jkbk2004 closed this Apr 14, 2023
@jkbk2004 jkbk2004 reopened this Apr 14, 2023
@WenMeng-NOAA
Copy link
Copy Markdown
Contributor Author

@WenMeng-NOAA we can resolve the conversations and merge in this pr. can you resolve?

Yes, all conversations are solved.

@jkbk2004
Copy link
Copy Markdown
Collaborator

@WenMeng-NOAA I will let you create an issue to clean up code. merging the pr now.

@jkbk2004 jkbk2004 merged commit f071fc6 into NOAA-EMC:develop Apr 14, 2023
@WenMeng-NOAA WenMeng-NOAA deleted the upp_apr branch March 6, 2025 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done
Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants