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

Updates on the test cases and minor fixes in the rt* scripts #68

Open
wants to merge 33 commits into
base: feature/coastal_app
Choose a base branch
from

Conversation

pvelissariou1
Copy link
Collaborator

@pvelissariou1 pvelissariou1 commented Apr 17, 2024

  • Added new test cases for SCHISM (hurricane Sandy)
  • Moved the SCHISM Ike cases to Idealized
  • Updated the test configuration files in tests/tests
  • Minor fixes in the rt* files

Input files: /work/noaa/nosofs/pvelissa/RT-03132024/NEMSfv3gfs/develop-20240415

@pvelissariou1 pvelissariou1 self-assigned this Apr 17, 2024
Copy link
Collaborator

@uturuncoglu uturuncoglu left a comment

Choose a reason for hiding this comment

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

@pvelissariou1 I just review part of it and put some comments. I am working on sync and updating SCHSIM submodules. I'll update you once I have updated model and then you could sync your branch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pvelissariou1 @janahaddad I think we will move this UFS Coastal specific readme file to app level (ufs-coastal-app). Right. This also includes the images. BTW, I am not comfortable to use image files in the documentation at least in the README file. This will make our life harder when we need to change those information and we will need to recreate those files. I think it is better to go with plain text. Anyway let me know what you think.

Copy link
Collaborator

@uturuncoglu uturuncoglu Apr 18, 2024

Choose a reason for hiding this comment

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

I think if we move UFS coastal README to app level then we need to revert back the name of this file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am syncing both SCHSIM and SCHSIM-ESMF at this point and pointing their master branch. So, once I finish testing that and syncing ufs-coastal with ufs-weather-model, this branch needs to be synced. Then, these will disappear from the PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, I think we need to get rid of entire images folder. They seems also belong to old version of the model (CoastalApp).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is not required. If we have actions in the app level we could show their status dynamically in the README page.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't need to adjust these. I am currently syncing ufs-coastal and if there is a still issue with those queue names. We could open an issue in ufs-weather-model level and make the changes over there. We might also adjust rt.sh to allow using user specified queue names from command line rather than hardcoding in her. That will bring more flexibility to the RT system.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is better to keep only fully tested configurations in the rt_coastal.conf. It would be nice to create another file (rt_coastal_tmp.conf etc.) for the rest.

@pvelissariou1
Copy link
Collaborator Author

@uturuncoglu I agree with most of your comments. Please go ahead and sync with ufs-weather-model and schism/schism-esmf and then we can re-visit the PR. In the meantime I am working locally on hercules ufs-coastal with updated schism/schism-esmf and setting/configuring/testing the various test cases. I will separate the "fully tested" in the rt_coastal.conf and move the rest to something like rt_coastal_testing.conf. At this point I am considering ADCIRC cases as partially tested and the rest FVCOM/ROMS as not tested.

@uturuncoglu
Copy link
Collaborator

@pvelissariou1 Okay. I pointed to the SCHSIM master for both model and cap and I am seeing answer changes in atm2sch2ww3 (atm2sch is passing). This could be due to having old baseline on Hercules but I am double checking the output to be sure. Once it is done and seems okay, I'll update the baseline for atm2sch2ww3 on Hercules. then move to sync with uas-weather-model. I'll keep you posted. Thanks again for all your hard work.

@pvelissariou1
Copy link
Collaborator Author

@uturuncoglu When you have synchronized everything I will re-run the tests from my side and if everything is ok we can merge the RT folder as well.

@uturuncoglu
Copy link
Collaborator

@pvelissariou1 I am still working on it. I'll update you soon. As a side note, I am also working on another PR in ufs-weather-model side related with the land project but I'll try to sync soon. I need to check the result of the wave test to be sure it is workin to update the baseline.

@pvelissariou1
Copy link
Collaborator Author

@uturuncoglu Ufuk, please take your time. I am good with my ufs-coastal clone (I have updated all SCHISM related sources). I am working on multiple test cases now. Tomorrow we might need to have a meeting to discuss my updates.

@uturuncoglu
Copy link
Collaborator

@pvelissariou1 Thanks. I am trying to compile the SCHSIM utils on Hercules but getting error like /usr/bin/ld: /work/noaa/epic/role-epic/spack-stack/hercules/spack-stack-1.5.1/envs/unified-env/install/intel/2021.9.0/scotch-7.0.4-vclm5gy/lib/libptscotch.a(dgraph_fold_comm.c.o): undefined reference to symbol '__svml_idiv4' it was fine wen I run the RT. Anyway, I am trying to resolve it at this point. Probably it is related with my environment.

@pvelissariou1
Copy link
Collaborator Author

@uturuncoglu I am able to compile SCHISM within ufs-coastal and standalone without issues. Scotch is not used.
(that is, not the latest commit of SCHISM but the previous one). If you want I can check again on Hercules.

@uturuncoglu
Copy link
Collaborator

@pvelissariou1 In my case I am compiling WW3 coupled configurations and I think WW3 is using Scotch. Anyway, I am trying to do fresh install now.

@pvelissariou1
Copy link
Collaborator Author

@uturuncoglu Same here, SCHISM+WW3 compilations are working just fine for me. Using Scotch (WW3) and ParMETIS (SCHISM).

@uturuncoglu
Copy link
Collaborator

@pvelissariou1 @janahaddad @yunfangsun @saeed-moghimi-noaa FYI, I did following,

  • I updated SCHSIM and point master branches for both model and coupling interface. In my testing I found an issue with SCHSIM running under debug mode. You could find more information in the following link, The coupled debug test is failing schism-esmf#5. I'll look at the issue later.

  • I also tried to sync the ufs-coastal with ufs-weather-model head of develop. But, I am getting following error,

16:
16:  *** WAVEWATCH III ERROR IN W3IOGR :
16:      ERROR IN READING FROM mod_def.ww3 FILE
16:      IOSTAT =   67     MOD DEF FILE WAS GENERATED WITH A DIFFERENT
16:      WW3 VERSION OR USING A DIFFERENT SWITCH FILE.
16:      MAKE SURE WW3_GRID IS COMPILED WITH SAME SWITCH
16:      AS WW3_SHEL OR WW3_MULTI, RUN WW3_GRID AGAIN
16:      AND THEN TRY AGAIN THE PROGRAM YOU JUST USED.
16:
16:

I think we need to run ww3_grid for all the configurations with WW3. Any idea? I also found that I was syncing our WW3 fork (and branch) with NOAA-EMC WW3 develop branch by mistake. Actually dev/ufs-weather-model branch is used by the UFS Weather Model. So, I cherry pick our additional development in WW3 side (such as bringing radiation stress support, making standalone build possible) and create another branch in our fork with those changes. The new branch in WW3 side is dev/ufs-coastal (we could change its name later if we need). I am plaining to point that one after I regenerate the file and resolve the issue. So, I did not push the synced ufs-coastal yet.

  • I also split tested configurations from the fully tested ones vs. still testing phase. So, now rt_coastal.conf just includes tests that are known as fine (the ones indicated as bold in the wiki page: https://github.com/oceanmodeling/ufs-coastal/wiki/Current-Status-of-UFS%E2%80%90Coastal-Implementation). So, if you have other configurations that are tested extensively and has baseline on Hercules and Orion (they need to reproduce their self), then please add them to the rt_coastal.conf by following the convention in the file. The old version of rt_coastal.conf is now renamed as rt_coastal_testing.conf. So, maybe we could delete the tests from there that is already defined in rt_coastal.conf. I think with this way we will be sure that all the tests found in rt_coastal.conf will work on regularly tested platforms (except minor issues that we need to solve soon).

@pvelissariou1
Copy link
Collaborator Author

@uturuncoglu Ufuk thank you for the update. Regarding rt_costal.conf file I have done the same thing as I am trying to finalize all SCHISM, WW3 cases, let me take care of that at this moment. Regarding WW3 I'll keep testing all cases SCHISM+WW3, WW3 and I'll report back. In the shinnecock test cases for ww3 the mod_def* files are quite old but I don't understand why you are getting this error message now. Anyway, I think we need to re-adjust the job submission scripts to generate these files on the fly as we are doing in CoastalApp-testsuite. To generate these files on the fly is standard approach used by the WW3 folks as well.

@uturuncoglu
Copy link
Collaborator

@pvelissariou1 Okay. Once I run ww3_grid, the case is fine. I am not sure why we ned this now but it could be related with my fault by using wrong branch to update our fork. Anyway, I'll update files for the case that has ww3 component. If you have same issue, you might need to run ww3_grid in your side too. The RT basically did not designed to run different tasks except running build and run. Of course it would be extended but I think there is no need at this point and also NOAA-EMC is trying to modernize the RT framework at this point. If you think that it is really needed, then the best way is to open an issue in UFS Weather Model side about it and start discussion.

@uturuncoglu
Copy link
Collaborator

@pvelissariou1 Okay, I generated all files again and running all RTs now. If they passes, i'll also merge the staff related with sync. Keep you updated.

@pvelissariou1
Copy link
Collaborator Author

@uturuncoglu , @janahaddad Let's put this on the backlog for the time being, it is not urgent. In the future we will talk about this again, do we need to have this functionality on the RT side or, on the app side. Models ADCIRC, WW3, etc. require some kind of pre-processing steps before the actual run. I agree RTs should be in frozen status as we use them to test things out.

@pvelissariou1
Copy link
Collaborator Author

@uturuncoglu Great, thanks

@uturuncoglu
Copy link
Collaborator

@pvelissariou1 @janahaddad @saeed-moghimi-noaa @yunfangsun I also synced the model with ufs-weather-model head of develop. I was syncing our WW3 fork with NOAA-EMC WW3 develop branch by mistake. Actually dev/ufs-weather-model branch is used by the UFS Weather Model. So, I cherry pick our additional development in WW3 side (such as bringing radiation stress support, making standalone build possible) and create another branch in our fork with those changes. Also, fixed the issue related with the standalone WW3 configuration. Now, our branch is based on the one used by the ufs-weather-model. The new branch in WW3 side is dev/ufs-coastal (we could change its name later if we need) and ufs-coastal now pointing that one.

  • There are two failed case in the baseline check on Hercules when you run all the test in rt_coastal.conf. Those are ROMS and FVCOM ones. We have ticket for both of them opened in the past. FVCOM one is RT baseline check is failing for coastal_scituateharbor_atm2fvc FVCOM#2 and ROMS one is RT coastal_irene_atm2roms is not reproducing on Frontera roms#3. The ROMS one is little bit weird. The NCAR cprnc tool shows the data identical with baseline but UFS uses nccmp command. So, I'll look at it further.

  • Also, note that now you need to combine test name with compiler when you run the regression test. Something like following: ./rt.sh -l rt_coastal.conf -a [your account] -k -n "coastal_ike_shinnecock_atm2sch2ww3 intel". I also confirmed that providing -e to the rt.sh will allow to run the test in parallel. So, you could try ./rt.sh -a nems -l rt_coastal.conf -e. The tests that are in rt_coastal.conf file passes without any issue (except known minor issues that we have tickets).

  • I think we also need to update app level documentation to reflect the changes in the rt.sh.

Anyway, please sync your fork with current version of ufs-coastal and please also resolve the outstanding issues if there are. It would be nice to run all the tests (incl. newly added ones) in rt_coastal.conf one more time and create baseline for those new test before merge to be sure that they are still working. Then, I think we will be ready to merge it.

I think as a part of this PR, we also need to update the input directory. So, please let me know about it. We need to sync it across platforms like Orion, Hercules and also Frontera. In the meantime, let me know if you have any issue.

@janahaddad
Copy link
Collaborator

Just catching up with this but thank you @uturuncoglu for this. We'll have time to review this PR at tomorrow's meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs review
Development

Successfully merging this pull request may close these issues.

3 participants