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.

Sign up for GitHub

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

Jump to bottom

Updated flake physics, suites and the related files in tests directory #506

Closed
wants to merge 29 commits into from
Closed

Updated flake physics, suites and the related files in tests directory #506

wants to merge 29 commits into from

Conversation

YihuaWu-NOAA
Copy link
Contributor

@YihuaWu-NOAA YihuaWu-NOAA commented Apr 2, 2021 •
edited by BrianCurtis-NOAA
Loading

PR Checklist

  • Ths PR is up-to-date with the top of all sub-component repositories except for those sub-components which are the subject of this PR. Please consult the ufs-weather-model wiki if you are unsure how to do this.

  • This PR has been tested using a branch which is up-to-date with the top of all sub-component repositories except for those sub-components which are the subject of this PR

  • An Issue describing the work contained in this PR has been created either in the subcomponent(s) or in the ufs-weather-model. The Issue should be created in the repository that is most relevant to the changes in contained in the PR. The Issue and the dependent sub-component PR
    are specified below.

  • If new or updated input data is required by this PR, it is clearly stated in the text of the PR.

Instructions: All subsequent sections of text should be filled in as appropriate.

The information provided below allows the code managers to understand the changes relevant to this PR, whether those changes are in the ufs-weather-model repository or in a subcomponent repository. Ufs-weather-model code managers will use the information provided to add any applicable labels, assign reviewers and place it in the Commit Queue. Once the PR is in the Commit Queue, it is the PR owner's responsiblity to keep the PR up-to-date with the develop branch of ufs-weather-model.

Description

Provide a detailed description of what this PR does. What bug does it fix, or what feature does it add? Is a change of answers expected from this PR? Are any library updates included in this PR (modulefiles etc.)?

Issue(s) addressed

Resolves #512

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 regression tests and unit tests (utests) been run? On which platforms and with which compilers? (Note that unit tests can only be run on tier-1 platforms)

  • hera.intel
  • hera.gnu
  • orion.intel
  • cheyenne.intel
  • cheyenne.gnu
  • gaea.intel
  • jet.intel
  • wcoss_cray
  • wcoss_dell_p3

Dependencies

Do PRs in upstream repositories need to be merged first?
If so add the "waiting for other repos" label and list the upstream PRs
Depends NOAA-EMC/fv3atm#270
Depends NCAR/ccpp-physics#588

@BrianCurtis-NOAA
Copy link
Collaborator

Yihua, Thanks for submitting the new PR.
There are a few things I had questions about:
-Is there a UFS issue you've created already for this PR? If not please create one.
-Is there a PR created in FV3 with the changes seen in your FV3 repo? (is there any other upstream PR's?)
-Once you are done testing, please make sure to move the rt.conf_* file information into rt.conf.
-Please make sure you double check you have all pertinent information in the description above, as it helps us make sure this Pr is ready for review/merging.

Thanks for your time.

@BrianCurtis-NOAA BrianCurtis-NOAA mentioned this pull request Apr 2, 2021
Closed
@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 2, 2021 via email

@BrianCurtis-NOAA
Copy link
Collaborator

@YihuaWu-NOAA

Tasks:

  • We require that all PR's are associated with an issue. Please create an issue in ufs-weather-model and fill out the template provided.
  • Also, please make edits on the top of the PR to fill out the sections of the template. For example the description section needs to be filled out.
  • Lastly, I see that the MOM6 submodule is not pointing to the head of MOM6 (where ufs-weather-model is). Please make sure to update all of your submodules to those in ufs-weather-model unless you're specifically testing with a fork.

Question: The previous PR mentions changes to MOM6. Were these already made or is there a PR in MOM6 to be linked here as well?

Thanks for your time.

@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 8, 2021 via email

@climbfuji
Copy link
Collaborator

Hi, Brian, I still have some questions about the checklists and the check errors. The PR was created 6 days ago. It was up-to-date with the top of all sub-component repositories at that time. Now it is one commit behind. Should I update my code? Should I run the regressions test with the latest code again? I did not touch the code related FMS https://github.com/NOAA-GFDL/FMS. Why did it cause errors in my PR? Thank you very much! Yihua----------------------------------------------------Yihua Wu, Ph DI.M. Systems Group NOAA/NWS/NCEP/EMC 5830 University Research Court, Room: 2031 College Park, Maryland 20740, USA Phone: (301)-683-3691 Fax: (301)-683-3703
…

@YihuaWu-NOAA My position is that you had your code up to date and you had successful tests. I would wait until it is your turn in the commit queue, then update all repositories again and do the regression testing. Of course you need to be fluent in github procedures in order to update all your branches and submodule pointers correctly. If you think that might require some back and forth, it would indeed be a good idea to do it now and then have a fresh memory and notes when it is your turn in the commit queue. My 2c.

@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 8, 2021 via email

@BrianCurtis-NOAA
Copy link
Collaborator

BrianCurtis-NOAA commented Apr 8, 2021 •
edited
Loading

@YihuaWu-NOAA
Are you trying to create new tests or use old ones?
Why I ask is looking through your rt.conf, we've changed the format a bit. For example if you're creating tests your changes may look like the following depending on what tests you are creating:

COMPILE | APP=ATM SUITES=FV3_GFS_v15_thompson_mynn,FV3_GFS_v15_thompson_mynn_noahmp,FV3_GFS_v15_thompson_mynn_noah_flake,FV3_GFS_v15_thompson_mynn_noahmp_flake 32BIT=Y | | fv3 |
RUN     | fv3_regional_control | | fv3 |
RUN     | fv3_regional_mynn_noahmp | | fv3 |
RUN     | fv3_regional_mynn_noah_flake | | fv3 |
RUN     | fv3_regional_mynn_noahmp_flake | | fv3 |

If you're creating new tests we would need to create new baselines for those tests as well.

Do you anticipate any of your changes to impact the RT baselines?

@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 8, 2021 via email

@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 8, 2021 via email

@BrianCurtis-NOAA
Copy link
Collaborator

@YihuaWu-NOAA Agreed, if you're not going to be creating any new tests, then could you please remove the rt.conf_* files and revert your changes to rt.conf?

@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 9, 2021 via email

@BrianCurtis-NOAA BrianCurtis-NOAA added the No Baseline Change No Baseline Change label Apr 12, 2021
@BrianCurtis-NOAA
Copy link
Collaborator

BrianCurtis-NOAA commented Apr 12, 2021 •
edited
Loading

@YihuaWu-NOAA , Its your turn in the queue. So things will speed up here.

We need you to get your UFS repo and submodules in CCPP and FV3 updated with the latest changes.

@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 12, 2021 via email

@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 12, 2021 via email

@BrianCurtis-NOAA
Copy link
Collaborator

@YihuaWu-NOAA it looks like you didn't fix your conflicts in CCPP Physics: NCAR/ccpp-physics@334e245

For example:

<<<<<<< HEAD
     &       pi, sbc, ps, u1, v1, t1, q1, tref, cm, ch,                 &
     &       prsl1, prslki, prsik1, prslk1, wet, lake, xlon, sinlat,    &
=======
     &       pi, tgice, sbc, ps, u1, v1, t1, q1, tref, cm, ch,                 &
     &       prsl1, prslki, prsik1, prslk1, wet, xlon, sinlat,          &
>>>>>>> 990b9d0416e340e40e4b136561a85f63d46671e2

from <<<<<<< HEAD to =======, is the code in your repo
from ======= to >>>>>>> 990b9d, is the code from the upstream repo

You need to go through each part of the code where you see that and keep the section of code that you want to keep in the repo. Then get rid of the <<<<<<< HEAD, =======, >>>>>>> 990b... identifiers.

@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 12, 2021 via email

@BrianCurtis-NOAA
Copy link
Collaborator

Looks good on the submodule side. Just waiting on bringing this branch up to match ufs-community/ufs-weather-model

@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 12, 2021 via email

@BrianCurtis-NOAA BrianCurtis-NOAA removed the Waiting for Reviews The PR is waiting for reviews from associated component PR's. label Apr 14, 2021
@BrianCurtis-NOAA
Copy link
Collaborator

@YihuaWu-NOAA We are going to move this PR back a little bit while we figure out what is going on.

I've tried the latest branch and the test still fails upon comparison with baselines.

@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 14, 2021 via email

@BrianCurtis-NOAA
Copy link
Collaborator 

cd ufs-weather-model/tests
cp rt.conf rt.test

in rt.test Remove everything but the compile line and the test (RUN) line for the nsst test

export RT_COMPILER='intel' && ./rt.sh -e -l rt.test > rt.out 2>&1

you can tail -f rt.out if you want to watch how it's progressing, or watch squeue --user=<user name> (or bjobs if you use WCOSS)

@DeniseWorthen
Copy link
Collaborator

@YihuaWu-NOAA There are multiple fields which differ in the phyf000.tileX.nc files between your PR and the existing baseline (20210406) at the time we attempted to complete your PR. See this directory on orion:

/work/noaa/stmp/dworthen/stmp/dworthen/FV3_RT/v16nsst.

The subdirectory pr506 is a copy of the run directory which was created when running the regression tests for your PR. In other words, it is the same run directory you would get by using the command Brian gave you above.

The second subdirectory rt_108260 was created by doing the same thing for the develop branch of ufs-weather.

Comparing the phyf000.tileX.nc files between these two run directories show that the ATM does not appear to be initializing the same way in your PR. For example, this is the difference field in tmpsfc in the phyf000.tile3.nc file. All the differences appear in the the ocean.

Screen Shot 2021-04-15 at 11 36 48 AM

You can copy the two run directories to your own space. Then make your changes in your code, compile (see this page) and copy the executable into your copy of the pr506 directory. You can run the new executable using sbatch job_card. You'll need to just debug your branch until you find no differences between the model forecast files.

@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 15, 2021 via email

@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 15, 2021 via email

@climbfuji
Copy link
Collaborator

climbfuji commented Apr 15, 2021 via email

@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 15, 2021 via email

@BrianCurtis-NOAA
Copy link
Collaborator

No, the new baseline date is correct.

@XuLi-NOAA
Copy link
Contributor

The regression test is successful with this baseline: baseline dir = /scratch1/NCEPDEV/nems/emc.nemspara/RT/NEMSfv3gfs/develop-20210414/INTEL/cpld_bmarkfrac_v16_nsst Should I run with the baseline dated 20210406? Thanks! Yihua----------------------------------------------------Yihua Wu, Ph DI.M. Systems Group NOAA/NWS/NCEP/EMC 5830 University Research Court, Room: 2031 College Park, Maryland 20740, USA Phone: (301)-683-3691 Fax: (301)-683-3703 On Thu, Apr 15, 2021 at 5:57 PM Dom Heinzeller @.> wrote:
…
At this point, the order doesn't matter, the metadata file can list the variables in different order. But in the near future it will, and I am currently creating a PR to detect and fix all those out-of-order cases. But for your PR it has no impact. > On Apr 15, 2021, at 3:54 PM, YihuaWu-NOAA @.> wrote: > > > Hi, Brian and Denise, > > I think this might be the problem: in sfc_nst.meta file, the order of sbc, > pi, tgice, should be pi, tgice, sbc. But, it was sbc, pi, tgice. > This was caused during merging processes. I already updated this on > github, and a test was submitted. > Thanks! > > > > > Yihua----------------------------------------------------Yihua Wu, Ph DI.M. > Systems Group > NOAA/NWS/NCEP/EMC > 5830 University Research Court, Room: 2031 > College Park, Maryland 20740, USA > Phone: (301)-683-3691 > Fax: (301)-683-3703 > > > On Thu, Apr 15, 2021 at 12:33 PM Yihua. Wu - NOAA Affiliate < > @.> wrote: > > > Hi, Denise, > > > > Thanks! I will investigate this. > > > > > > > > > > Yihua----------------------------------------------------Yihua Wu, Ph DI.M. > > Systems Group > > NOAA/NWS/NCEP/EMC > > 5830 University Research Court, Room: 2031 > > College Park, Maryland 20740, USA > > Phone: (301)-683-3691 > > Fax: (301)-683-3703 > > > > > > On Thu, Apr 15, 2021 at 11:46 AM Denise Worthen @.> > > wrote: > > > >> @YihuaWu-NOAA https://github.com/YihuaWu-NOAA There are multiple > >> fields which differ in the phyf000.tileX.nc files between your PR and > >> the existing baseline (20210406) at the time we attempted to complete your > >> PR. See this directory on orion: > >> > >> /work/noaa/stmp/dworthen/stmp/dworthen/FV3_RT/v16nsst. > >> > >> The subdirectory pr506 is a copy of the run directory which was created > >> when running the regression tests for your PR. In other words, it is the > >> same run directory you would get by using the command Brian gave you above. > >> > >> The second subdirectory rt_108260 was created by doing the same thing > >> for the develop branch of ufs-weather. > >> > >> Comparing the phyf000.tileX.nc files between these two run directories > >> show that the ATM does not appear to be initializing the same way in your > >> PR. For example, this is the difference field in tmpsfc in the > >> phyf000.tile3.nc file. All the differences appear in the the ocean. > >> > >> [image: Screen Shot 2021-04-15 at 11 36 48 AM] > >> < https://user-images.githubusercontent.com/40498404/114897041-df532200-9dde-11eb-98f1-caa9eba4b058.png > > >> > >> You can copy the two run directories to your own space. Then make your > >> changes in your code, compile (see this page > >> < https://github.com/ufs-community/ufs-weather-model/wiki/Building-model>) > >> and copy the executable into your copy of the pr506 directory. You can > >> run the new executable using sbatch job_card. You'll need to just debug > >> your branch until you find no differences between the model forecast files. > >> > >> — > >> You are receiving this because you were mentioned. > >> Reply to this email directly, view it on GitHub > >> < #506 (comment) >, > >> or unsubscribe > >> < https://github.com/notifications/unsubscribe-auth/ANJBYQELZLBPFOSDB2NMBWTTI4C6RANCNFSM42I5ZU4A > > >> . > >> > > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub < #506 (comment)>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AB5C2RNTAWULHVRVMFI2NXTTI5OABANCNFSM42I5ZU4A >. > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#506 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANJBYQBQBPY7JLW4PLUQRDDTI5OODANCNFSM42I5ZU4A .

@YihuaWu-NOAA : Nice to know the RT went through! Can you summarize what changes you did to make this happen?

@BrianCurtis-NOAA
Copy link
Collaborator

It's good that it's passed the test! We are unsure what happened with ccpp-physics branch though. All of the sudden there were 40 files edited, look at https://github.com/NCAR/ccpp-physics/pull/588/files . Maybe I'm mistaken, but I recall there being only a few files changed there.

@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 16, 2021 via email

@BrianCurtis-NOAA
Copy link
Collaborator

BrianCurtis-NOAA commented Apr 16, 2021 •
edited
Loading

in your FV3 PR, ccpp-physics points to hash 26aa3d6, your actual ccpp-physics branch has hash of 59ab0cd. Make sure that every time you update your ccpp-physics branch (this creates a new hash) that you also update your FV3 branch to point to the updated ccpp-physics branch hash.

@climbfuji
Copy link
Collaborator

in your FV3 PR, ccpp-physics points to hash 26aa3d6, your actual ccpp-physics branch has hash of 59ab0cd. Make sure that every time you update your ccpp-physics branch (this creates a new hash) that you also update your FV3 branch to point to the updated ccpp-physics branch hash.

But this doesn't change that the ccpp-physics PR shows 40 changed files? That PR needs to be fixed first.

@DeniseWorthen
Copy link
Collaborator

@YihuaWu-NOAA Please try the following:

git clone https://github.com/YihuaWu-NOAA/fv3atm.git
git checkout flake
git submodule update --init --recursive
cd ccpp/physics
git checkout flake
cd ../.. (shows ccpp/physics(new commits))
git add ccpp/physics
git commit

now, add upstream remote to check differences:
git remote add upstream https://github.com/NOAA-EMC/fv3atm.git
git fetch upstream
git diff upstream/develop

shows:

diff --git a/ccpp/physics b/ccpp/physics
index 26aa3d6..59ab0cd 160000
--- a/ccpp/physics
+++ b/ccpp/physics
@@ -1 +1 @@
-Subproject commit 26aa3d6
+Subproject commit 59ab0cd

@YihuaWu-NOAA
Copy link
Contributor Author

YihuaWu-NOAA commented Apr 16, 2021 via email

@climbfuji
Copy link
Collaborator

Replaced by #532.

@climbfuji climbfuji closed this Apr 19, 2021
epic-cicd-jenkins pushed a commit that referenced this pull request Apr 17, 2023
@chan-hoo
Make the name of the forecast model the same as its repository (#506)
1393105 
* Match the name of forecast model with repo

* Modify the default name of FCST_MODEL
epic-cicd-jenkins pushed a commit that referenced this pull request Apr 17, 2023
@EdwardSnyder-NOAA
[develop] Added logic to not include slurm memory option for noaacloud (
53c62ca 
#506)

Co-authored-by: Edward Snyder <Edward.Snyder@noaa.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@YihuaWu-NOAA YihuaWu-NOAA

@BrianCurtis-NOAA BrianCurtis-NOAA

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update flake physics
6 participants
@YihuaWu-NOAA @BrianCurtis-NOAA @climbfuji @DeniseWorthen @junwang-noaa @XuLi-NOAA