Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update in-line post for RRFS and GFS #641

Merged
merged 13 commits into from
Apr 14, 2023
Merged

Conversation

WenMeng-NOAA
Copy link
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
io/post_fv3.F90 Outdated Show resolved Hide resolved
io/post_fv3.F90 Outdated Show resolved Hide resolved
io/post_fv3.F90 Outdated Show resolved Hide resolved
io/post_fv3.F90 Outdated Show resolved Hide resolved
@WenMeng-NOAA
Copy link
Contributor Author

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

@WenMeng-NOAA
Copy link
Contributor Author

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

io/post_fv3.F90 Outdated Show resolved Hide resolved
io/post_fv3.F90 Outdated Show resolved Hide resolved
@WenMeng-NOAA
Copy link
Contributor Author

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

io/post_fv3.F90 Outdated Show resolved Hide resolved
io/post_fv3.F90 Outdated Show resolved Hide resolved
io/post_fv3.F90 Outdated Show resolved Hide resolved
@DusanJovic-NOAA
Copy link
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
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
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
Contributor Author

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

@jkbk2004
Copy link
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
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
Collaborator

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

@jkbk2004
Copy link
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
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants