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

Fix OMP for scrip_remap_conservative MPI update #1

Merged
merged 10 commits into from
Dec 6, 2024

Conversation

MatthewMasarik-NOAA
Copy link

Pull Request Summary

Fixes statement ordering in OMP DO blocks for scrip_remap_conservative.F MPI PR.

Description

  • Provide a detailed description of what this PR does.
    • PR update scrip_remap_conservative to use MPI NOAA-EMC/WW3#1268 updates file scrip_remap_conservative.F to be optionally parallelized under MPI using the switch SCRIPMPI. Two OMP DO sections in this PR branch caused the build to fail while running ww3_ufs1.x regtests. The source of the problem is that a fortran do statement must directly follow the OMP DO directive. This sub-PR makes small re-ordering edits in two locations to satisfy that requirement.
  • What bug does it fix, or what feature does it add?
    • Bug fix for OMP functionality.
  • Is a change of answers expected from this PR?
    • No.

Please also include the following information:

  • Add any suggestions for a reviewer
  • Mention any labels that should be added:
    • bug.
  • Are answer changes expected from this PR? Please describe the changes and the reason why in addition to which of the following labels would apply: mod_def change, out_grd change, out_pnt change, restart file change, Regression test
    • No.

Issue(s) addressed

Commit Message

Fix OMP for scrip_remap_conservative MPI update

Check list

Testing

  • How were these changes tested?
    • regtest matrix.
  • Are the changes covered by regression tests? (If not, why? Do new tests need to be added?)
    • Yes. Regtests caught the issue.
  • Have the matrix regression tests been run (if yes, please note HPC and compiler)?
    • Yes. Hera / Intel.
  • Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.)
    • N/A.
  • Please provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt):
**********************************************************************      
********************* non-identical cases ****************************      
**********************************************************************      
mww3_test_03/./work_PR1_MPI_e                     (1 files differ)          
mww3_test_03/./work_PR3_UNO_MPI_e                     (1 files differ)      
mww3_test_03/./work_PR2_UQ_MPI_e                     (1 files differ)       
mww3_test_03/./work_PR2_UNO_MPI_e                     (1 files differ)      
mww3_test_03/./work_PR2_UNO_MPI_d2                     (17 files differ)    
mww3_test_03/./work_PR1_MPI_d2                     (8 files differ)         
mww3_test_03/./work_PR3_UNO_MPI_d2_c                     (16 files differ)  
mww3_test_03/./work_PR3_UQ_MPI_d2_c                     (16 files differ)   
mww3_test_03/./work_PR3_UNO_MPI_d2                     (17 files differ)    
mww3_test_03/./work_PR2_UQ_MPI_d2                     (16 files differ)     
mww3_test_03/./work_PR3_UQ_MPI_e                     (1 files differ)       
mww3_test_03/./work_PR3_UNO_MPI_e_c                     (1 files differ)    
mww3_test_03/./work_PR3_UQ_MPI_d2                     (15 files differ)     
mww3_test_09/./work_MPI_ASCII                     (0 files differ)          
ww3_tp2.10/./work_MPI_OMPH                     (7 files differ)             
ww3_tp2.16/./work_MPI_OMPH                     (4 files differ)             
ww3_tp2.6/./work_ST4_ASCII                     (0 files differ)             
ww3_ufs1.3/./work_a                     (3 files differ)                    
                                                                            
**********************************************************************      
************************ identical cases *****************************      
**********************************************************************

@jcwarner-usgs jcwarner-usgs merged commit cf3dc8f into jcwarner-usgs:develop Dec 6, 2024
@jcwarner-usgs
Copy link
Owner

ok sorry for the delay here.
I did a merge pull request, and i looked at the changes to the scrip_remap_conservative.F90. That all looks fine.
Honestly i dont use openmp anymore, so i had not tested that. and there was some white space removal. all good.
The changes look great. thanks.
Do i need to do anything else?

@MatthewMasarik-NOAA
Copy link
Author

ok sorry for the delay here. I did a merge pull request, and i looked at the changes to the scrip_remap_conservative.F90. That all looks fine. Honestly i dont use openmp anymore, so i had not tested that. and there was some white space removal. all good. The changes look great. thanks. Do i need to do anything else?

hi @jcwarner-usgs, the merge looks perfect, thanks for reviewing and committing it. I had ran the full matrix of regtests and the only one that got flagged was our ww3_ufs1.x using hybrid MPI/OpenMP. When I looked into it I found we actually had an issue with the OMPH switches for those tests. I had done testing for each of these, and thought they should be keep separate, so I put them in the two PRs. no worries on delay. I apologize this PR processing got dragged out as we had to take that pause. I think we are finally almost there.

I will have one small recommendation for the switches.json file entry which I'll post early next Monday (so we don't start in on something new 5pm on Friday!). After that I'll do the final testing, and if nothing unexpected comes up then we should be able to approve and merge it in by early next week. hope you have a great weekend.

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

Successfully merging this pull request may close these issues.

4 participants