BF OpenMp b4b

[ajhenrique]

Bugfix: Changes to several modules with OpenMP loops to declare properly PRIVATE variables and avoid spurious assignment of values that lead to b4b reproducibility issues. Expanded matrix_ncep to run hybrid MPI/OMP cases.

[ukmo-ccbunney]

@ajhenrique - It mostly looks good to me. I've left a few comments on specific lines.

[ukmo-ccbunney]

@ajhenrique There appears to be a lot of changes in the make_makefiles.sh file - more than just the change handling the mutual exclusion of T1 and OMPX switches. Was this intentional?

I.e. commit fb2b47b

[ajhenrique]

The integer STKBND_INDEX isn't used, https://github.com/ajhenrique/WW3/blob/456edd6d9bfb01528053cf5f11c2e1c7b9c72057/model/ftn/w3iogomd.ftn#L3683

Both NB and STKBND_INDEX have been deleted.

[mickaelaccensi]

I'm not expert in OMP so it would be better if Chris and Carsten can review this PR.

[ajhenrique]

I'm not expert in OMP so it would be better if Chris and Carsten can review this PR.

OK, agreed. Thanks @mickaelaccensi !

[ajhenrique]

@CarstenHansen @ukmo-ccbunney Apologies for leaving this hanging. My regtests had not completed and hung for unknown reasons (checking with sysadmins). I had to restart and decided to rerun to ensure results are correct. I hope to complete that today and provide the results so we can finalize this PR review. In the meantime, what are the pending issues that need to be addressed? Thanks!

[ukmo-ccbunney]

@ajhenrique - if the multi-switch version of w3adc works and allows you to overcome the T1/OMPH issue, then I am happy to accept the review.

[ajhenrique]

@ajhenrique - if the multi-switch version of w3adc works and allows you to overcome the T1/OMPH issue, then I am happy to accept the review.

Thanks @ukmo-ccbunney . Where should I expect to find an occurrence of a line with the double switch?

[ukmo-ccbunney]

@ajhenrique
To fix the issue in w3uqckmd.ftn where the T1 switch uses variables IX2, IY2 and QB that are not declared private in the OMP directive, you need to modify the lines around 880 to look like this:

!/OMPH/!$OMP PARALLEL DO PRIVATE (IP, IXY, CFL, IXYC, QB, IXYU, IXYD, &
!/OMPH/!/T1!$OMP                      QBO, QN, IX2, IY2,                   &
!/OMPH/!$OMP&                     DQ, DQNZ, QCN, QBN, QBR, CFAC )

Specifically the addition of the middle line. The modified w3adc program will then only insert the second line if both the T1 and OMPH switches are enabled.

Edit: QBO needs to be private too.

[CarstenHansen]

I still see two issues, which will probably not in practice appear very important:

1. Line 3749 in w3iogomd.ftn:
   !PRIVATE(JSEA,ISEA,FACTOR,KD,FKD,USSCO,MINDIFF,IB)
   I think the line is just a comment, where it was intented as a clause to the OMP PARALLEL DO in the line above.

2. The modified w3adc program seems to me as a real enhancement that will be useful in other contexts. I think the strings NEWLNE and OLDLNE to be read from the .ftn file could be declared somewhat longer than CHARACTER143, and limit the number of switches, to allow a future developer to write a line of 132 characters of free form Fortran code. For example, allow a max of four switches and declare CHARACTER176 NEWLNE, OLDLNE.

[ajhenrique]

Adding link to regtest matrix output from @ukmo-ccbunney for w3adc changes:
ajhenrique#3 (comment)

[ajhenrique]

Specifically the addition of the middle line. The modified w3adc program will then only insert the second line if both the T1 and OMPH switches are enabled.

Edit: QBO needs to be private too.

@ukmo-ccbunney can you confirm this also requires a similar change in w3uno2md.ftn near line 818:

!/OMPH/!$OMP PARALLEL DO PRIVATE (IP, IXY, CFL,          &
!/OMPH/!/T1!$OMP                   QBO, IY2, IX2, QN                   &
!/OMPH/!$OMP                      IXYC, IXYD, QB)  

[ajhenrique]

@CarstenHansen @ukmo-ccbunney I've completed the regression tests for this PR. I'd suggest that the request to change w3adc adding a limit to number of switches per line be made on a separate feature branch so we can move this PR in. Also, @mickaelaccensi would be making changes to w3iogo to address one of your comments.

Please revise your comments and let me know of you approve the PR. In attachment you can find the matrixCompSummary file reflecting my regtests.
matrix.zip

[ukmo-ccbunney]

@ukmo-ccbunney can you confirm this also requires a similar change in w3uno2md.ftn near line 818:

@ajhenrique : yes, and also IY and IX need to be private (line 834/835), i.e:

!/OMPH/!$OMP PARALLEL DO PRIVATE (IP, IXY, CFL,          &
!/OMPH/!/T1!$OMP                    QBO, IY, IX, IY2, IX2, QN             &
!/OMPH/!$OMP                      IXYC, IXYD, QB)  

[ukmo-ccbunney]

I'd suggest that the request to change w3adc adding a limit to number of switches per line be made on a separate feature branch so we can move this PR in
@ajhenrique - Agreed. I will raise a new PR with this change soon.

[CarstenHansen]

I'd suggest that the request to change w3adc adding a limit to number of switches per line be made on a separate feature branch so we can move this PR in
@ajhenrique - Agreed. I will raise a new PR with this change soon.

This sounds perfect.

[CarstenHansen]

@ajhenrique, I approve this PR

Please revise your comments and let me know of you approve the PR. In attachment you can find the matrixCompSummary file reflecting my regtests.

[ukmo-ccbunney]

Just one comment here, in response to your comment:
#160 (comment)

After that I think we are all ready to go.

[ajhenrique]

@ukmo-ccbunney @CarstenHansen thanks for the productive review process. Iĺl be pushing this up within the next hour. Let me know if there are any issue left out. @aliabdolali this PR is going in today.

[ukmo-ccbunney]

@ajhenrique Looks good to me!