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

Fb coupling time #9

Merged
merged 16 commits into from
Jul 30, 2020
Merged

Fb coupling time #9

merged 16 commits into from
Jul 30, 2020

Conversation

ukmo-juan-castillo
Copy link

Branch allowing a coupling time step different than the model time step. If no coupling time step is declared in the namelist, it will be made equal to the model time step after throwing a warning. The coupling time step must be a multiple of the model time step, otherwise the program will throw and error and stop.

A single, new regtest has been added in ww3_tp2.14 to test the new functionality. This has been tested in the ukmo_cray computer, for which a few changes had to be made in the regtests scripts to allow running these coupling test. A minor bug fix in the ice coupling regtests has also been fixed. The rest of the regtests also passed without problems.

The only thing missing would be adding a comment in the code informing of the changes, for which I would have to know the version for this branch (supposedly 7.08)

@ukmo-juan-castillo
Copy link
Author

I only left Chris and Andy as reviewers, more should be added once they are happy with the changes, see ticket 229:

NOAA-EMC#229

@ukmo-juan-castillo
Copy link
Author

One more modification that is possibly needed is updating the manual, explicitly saying what the default and valid values of the coupling time step are.

@ukmo-ccbunney ukmo-ccbunney requested review from aliabdolali and JessicaMeixner-NOAA and removed request for aliabdolali July 16, 2020 12:10
Copy link
Member

@ukmo-ccbunney ukmo-ccbunney left a comment

Choose a reason for hiding this comment

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

Just a small change request from me and a comment:
I see that you have included the coupling regtest (ww3_tp2.14) in the matrix.base.
I note that this was not part of the matrix.base before and I wonder if this was for a reason? @aliabdolali, @mickaelaccensi - are you able to comment on this?

regtests/bin/matrix_ukmo_cray Outdated Show resolved Hide resolved
Copy link

@ukmo-ansaulter ukmo-ansaulter left a comment

Choose a reason for hiding this comment

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

Thanks Juan,

Subject to the minor change suggested by @ukmo-ccbunney I'm happy with these changes.

Looking at the manual, the entry for OASIS it is a little thin, and I also don't think that an example of the namcouple namelist exists in the repository. It would be good to add these, but can I suggest as part of a new issue to review the OASIS manual entry more widely? Issue to be associated with the NOAA milestone: https://github.com/NOAA-EMC/WW3/milestone/22

As we are thinking of this change as a bugfix, the version of ww3_shel targeted should be the latest one

@ukmo-juan-castillo
Copy link
Author

For running the new regtests added in ww3_tp2.14, new input files would need to be added to the NOAA ftp site:

r-ww3.nc.OASACM2
r-toy.nc.OASACM2
toy_coupled_field.nc.OASACM2

It is sufficient if these files are a copy of the equivalent files for OASACM, although only 11 of the times contained in the files will be used.

@JessicaMeixner-NOAA
Copy link
Collaborator

For running the new regtests added in ww3_tp2.14, new input files would need to be added to the NOAA ftp site:

r-ww3.nc.OASACM2
r-toy.nc.OASACM2
toy_coupled_field.nc.OASACM2

It is sufficient if these files are a copy of the equivalent files for OASACM, although only 11 of the times contained in the files will be used.

If can use the existing files already on the ftp, you can just copy them to more than one location in the script here: https://github.com/NOAA-EMC/WW3/blob/develop/model/bin/ww3_from_ftp.sh#L44-L45 instead of adding duplications of the file on the ftp server.

@mickaelaccensi
Copy link
Collaborator

could you remove the option -c $cmplr from matrix.base since it's already defined in $rtst ?
In matrix_datarmor and matrix_ncep , cmplr is not an environment variable, we don't have an export cmplr.

echo "$rtst -s OASACM -w work_OASACM -c $cmplr -C OASIS -f -p $mpi -n $np -o netcdf $ww3 ww3_tp2.14" >> matrix.body
echo "$rtst -s OASACM2 -w work_OASACM2 -c $cmplr -C OASIS -f -p $mpi -n $np -o netcdf $ww3 ww3_tp2.14" >> matrix.body
echo "$rtst -s OASOCM -w work_OASOCM -c $cmplr -C OASIS -f -p $mpi -n $np -o netcdf $ww3 ww3_tp2.14" >> matrix.body
echo "$rtst -s OASICM -w work_OASICM -c $cmplr -C OASIS -f -p $mpi -n $np -o netcdf $ww3 ww3_tp2.14" >> matrix.body

@ukmo-juan-castillo
Copy link
Author

The latest suggested change has been implemented and tested. The trick to make everything work afterwards was setting by default the variable cmplOption to 'y' in matrix_ukmo_cray.

@mickaelaccensi
Copy link
Collaborator

mickaelaccensi commented Jul 22, 2020

Would you mind to remove this line from ww3_tp2.14/input/toy/prep_env.sh
sed -i "s/icc/gcc/g" cmplr
and add a new sed statement on wwatch3_cc like that :
sed -e "s/<optc_short>/$optcs/" -e "s/<optl_short>/$optls/" -e "s/<comp_mpi>/$comp_mpi/" -e "/<wwatch3_cc>/$WWATCH3_CC/" -e "s/<comp_mpi_exe>/$comp_mpi_exe/" cmplr.tmpl > cmplr
and add this at line18 (still in ww3_tp2.14/input/toy/prep_env.sh) (take care to add the quotes, it's interpreted here)
export WWATCH3_CC=grep WWATCH3_CC $WWATCH3_ENV | awk -F' ' '{print $2}'

then, change in ww3_tp2.14/input/oasis3-mct/util/make_dir/cmplr.tmpl :
CC = icc
by
CC = <wwatch3_cc>

@ukmo-juan-castillo
Copy link
Author

Latest changes suggested by the reviewer, which improve the coupling regtests scripts, have been implemented and tested.

@ukmo-ccbunney
Copy link
Member

@mickaelaccensi - Are you happy that the ww3_tp2.14 regtest is now included as part of the matrix.base? It was not included before - I just wanted to check if there was a reason for that or not (it is a more complex test case for certain!)

@ukmo-ccbunney
Copy link
Member

@ukmo-juan-castillo Can you please target the staging branch for this PR (not develop)? Thanks.

@ukmo-juan-castillo ukmo-juan-castillo changed the base branch from develop to staging July 23, 2020 11:05
@ukmo-ccbunney ukmo-ccbunney removed the request for review from aliabdolali July 23, 2020 12:59
@mickaelaccensi
Copy link
Collaborator

do you also have differences in OUTPUT_TOY.txt between this branch and develop branch ?

After decomp_def, il_paral = 1 4400 400
< After decomp_def, il_paral = 1 4800 400

and also differences in the values sent by the toy model, I don't know why.

in both tests, the APPLE partitionning is used but it provide different partitions. There is no changes in oasis3_mct directory neither in toymodel directory.

Do you see an explanation about it ?

@ukmo-juan-castillo
Copy link
Author

I will investigate these differences, thanks for pointing them out.

@ukmo-juan-castillo
Copy link
Author

The problem is that the output of the toy_model program is not bit reproducible. This happens because it is a MPI program, but the piece of code that writes to output does not take into account this fact.

I have two different solutions: one that writes just one output file, and other that writes one output file for each processor. I imagine you would like to implement here one of these fixes, which one would you prefer?

@ukmo-juan-castillo
Copy link
Author

I modified the toy program so that it only produces an output file for processor zero, as I believe this will be sufficient to check there are no differences between runs. I modified the matrix.comp script accordingly to stop comparing oasis logs and to remove time and location differences in the output.ww3 file (log file of WWIII when running in coupled mode).

@mickaelaccensi
Copy link
Collaborator

ww3_tp2.14 are bit4bit now. thanks a lot Juan for this work. I approve the PR

@ukmo-ccbunney
Copy link
Member

@JessicaMeixner-NOAA : are you happy with these changes? If so, I will get this PR signed off and merged. Thanks!

Copy link
Collaborator

@JessicaMeixner-NOAA JessicaMeixner-NOAA left a comment

Choose a reason for hiding this comment

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

I have never used the COU switch or any OASIS coupling, so I do not have any comments on that code or updates. If the OASIS coupling tests require extra libraries I think NCEP's matrix should turn these off. My only comment is that in the regression testing that its specified that these are OASIS coupling tests and that the dist=y be added in matrix.base to protect against those wanting to only run serial tests.

@@ -154,6 +154,8 @@ fi
export infgrv='y' # Second harmonic generation tests
export uost='y' # ww3_ts4 Unresolved Obstacles Source Term (UOST)
export assim='y' # Restart spectra update
export coup='y' # Atmosphere, ocean, and ice coupling
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there any secondary library requirements for running these tests? Also we should specify that this is OASIS coupling, not just "coupling".

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm asking about the requirements, because if there are any, I think this should be a 'n' for NCEP.

@@ -85,6 +85,7 @@
echo " echo ' Multi 07 (unstr. + reg. grds) : $multi07'" >> matrix.head
echo " echo ' Multi 08 (with ice) : $multi08'" >> matrix.head
echo " echo ' Assim Update of the restart file : $assim'" >> matrix.head
echo " echo ' Atmosphere, ocean, and ice coupling: $coup'" >> matrix.head
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer that we specify that these are OASIS coupling as there are multiple ways to couple and this is only for OASIS testing.

@@ -2003,6 +2004,16 @@
echo "$rtst -s ST4 -w work_UPD6_U_cap -i input_UPD6_U_cap $ww3 ww3_ta1" >> matrix.body
fi

#Test of atmosphere, ocean, and ice coupling
if [ "$coup" = 'y' ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since these are all MPI cases, we should make sure dist=y as well.

echo "$rtst -s OASOCM -w work_OASOCM -C OASIS -f -p $mpi -n $np -o netcdf $ww3 ww3_tp2.14" >> matrix.body
echo "$rtst -s OASICM -w work_OASICM -C OASIS -f -p $mpi -n $np -o netcdf $ww3 ww3_tp2.14" >> matrix.body
fi

# 365_day and 360_day calendars, no switch sharing here

if [ "$calendar" = 'y' ] && [ "$shrd" = 'y' ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not need the "$shrd"=y as there is not an alternative dist option.

Copy link
Author

Choose a reason for hiding this comment

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

I have modified the new regtests to specify that they are for OASIS coupling, and they will only run when dist='y'. All the dependencies are internal, i.e., all the libraries needed are already contained and compiled in the regtests without further external libraries. For that reason, I think it is a good idea to leave them in NCEP.

For the moment I leave out the modifications for calendar, as they belong to another ticket, and because imposing shrd='y' is only redundant. I can easily change it if you insist on modifying this here, though.

Copy link
Member

@ukmo-ccbunney ukmo-ccbunney left a comment

Choose a reason for hiding this comment

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

@ukmo-juan-castillo - Just a couple of things.

  1. A small addition to prep_env.sh script.
  2. I see that there is till a calendar = "y" entry in the regtest matrix files. We're you planning on keeping that? You comment above suggested you were removing it (as it relates to a different PR).

regtests/ww3_tp2.14/input/prep_env.sh Show resolved Hide resolved
remove reduntant condition in matrix.base for calendar tests
@ukmo-ccbunney ukmo-ccbunney merged commit 5c361aa into staging Jul 30, 2020
@ukmo-ccbunney
Copy link
Member

All merged - thanks @ukmo-juan-castillo.
Many thanks also to @JessicaMeixner-NOAA and @mickaelaccensi for their thorough reviews. 👍

@ukmo-juan-castillo ukmo-juan-castillo deleted the fb_coupling_time branch August 3, 2020 08:21
ukmo-ccbunney added a commit that referenced this pull request Aug 18, 2020
* Added boundary checks to the SMC grid input files for ww3_grid,
to ensure they comply with the limits of the nameslist.

* Fb 360 calendar (#8)

Changes to add support to 360-day and 365-day (no leap year) calendar - see ticket NOAA-EMC#209
  * Additional CALTYPE namelist parameter in MISC section
  * New ww3_tc1 regtest.

* RTD support for ww3_boun[dc] (#10)

* Updated ww3_bound and ww3_bounc to handle model grids formulated on a rotated pole.
* Manual and nml/inp files to updated clarify that ww3_bound/ww3_bounc only accept input spectra formulated on a standard pole grid.

* Fb coupling time (#9)

Updates to allow a coupling time step that is different from the model time step. 
* Includes new regtest (in ww3_tp2.14) for non-default oasis time step.
* ww3_tp2.14 regtest added to matrix.base.

* bug fix for ukmet development

Co-authored-by: lewis.sampson <[email protected]>
Co-authored-by: Juan Manuel Castillo Sanchez <[email protected]>
Co-authored-by: Chris Bunney <[email protected]>
ukmo-ccbunney added a commit that referenced this pull request Oct 21, 2020
* Added boundary checks to the SMC grid input files for ww3_grid,
to ensure they comply with the limits of the nameslist.

* Fb 360 calendar (#8)

Changes to add support to 360-day and 365-day (no leap year) calendar - see ticket NOAA-EMC#209
  * Additional CALTYPE namelist parameter in MISC section
  * New ww3_tc1 regtest.

* RTD support for ww3_boun[dc] (#10)

* Updated ww3_bound and ww3_bounc to handle model grids formulated on a rotated pole.
* Manual and nml/inp files to updated clarify that ww3_bound/ww3_bounc only accept input spectra formulated on a standard pole grid.

* Fb coupling time (#9)

Updates to allow a coupling time step that is different from the model time step. 
* Includes new regtest (in ww3_tp2.14) for non-default oasis time step.
* ww3_tp2.14 regtest added to matrix.base.

* bug fix for ukmet development

* see issue NOAA-EMC#245

this is a bug introduced with the last PR from UKMO
#8: 360 day climate calendar

in ww3_trnc.ftn
END CASE
should be replaced by
END SELECT

Co-authored-by: lewis.sampson <[email protected]>
Co-authored-by: Juan Manuel Castillo Sanchez <[email protected]>
Co-authored-by: Chris Bunney <[email protected]>
ukmo-ccbunney added a commit that referenced this pull request Jan 15, 2021
* Added boundary checks to the SMC grid input files for ww3_grid,
to ensure they comply with the limits of the nameslist.

* Fb 360 calendar (#8)

Changes to add support to 360-day and 365-day (no leap year) calendar - see ticket NOAA-EMC#209
  * Additional CALTYPE namelist parameter in MISC section
  * New ww3_tc1 regtest.

* RTD support for ww3_boun[dc] (#10)

* Updated ww3_bound and ww3_bounc to handle model grids formulated on a rotated pole.
* Manual and nml/inp files to updated clarify that ww3_bound/ww3_bounc only accept input spectra formulated on a standard pole grid.

* Fb coupling time (#9)

Updates to allow a coupling time step that is different from the model time step. 
* Includes new regtest (in ww3_tp2.14) for non-default oasis time step.
* ww3_tp2.14 regtest added to matrix.base.

* bug fix for ukmet development

* Updating develop with a change to allow code build using non-standard NetCDF libraries using alphabetical characters in their version name.

Co-authored-by: lewis.sampson <[email protected]>
Co-authored-by: Juan Manuel Castillo Sanchez <[email protected]>
Co-authored-by: Chris Bunney <[email protected]>
Co-authored-by: Ali Abdolali <[email protected]>
ukmo-ccbunney added a commit that referenced this pull request Apr 1, 2021
* Added boundary checks to the SMC grid input files for ww3_grid,
to ensure they comply with the limits of the nameslist.

* Fb 360 calendar (#8)

Changes to add support to 360-day and 365-day (no leap year) calendar - see ticket NOAA-EMC#209
  * Additional CALTYPE namelist parameter in MISC section
  * New ww3_tc1 regtest.

* RTD support for ww3_boun[dc] (#10)

* Updated ww3_bound and ww3_bounc to handle model grids formulated on a rotated pole.
* Manual and nml/inp files to updated clarify that ww3_bound/ww3_bounc only accept input spectra formulated on a standard pole grid.

* Fb coupling time (#9)

Updates to allow a coupling time step that is different from the model time step. 
* Includes new regtest (in ww3_tp2.14) for non-default oasis time step.
* ww3_tp2.14 regtest added to matrix.base.

* bug fix for ukmet development

* Periodicity fix for global unstructured grids

 - Corrects calculation of element areas and edge lengths across the
   dateline in w3triamd.ftn

 - Fixes calculation to determine whether a point is inside an element
   that spans the dateline in w3triamd.ftn

 - Corrects calculation of element centers and corners across dateline
   in wmscrpmd.ftn

 - Write out SCRIP file in netCDF format at the end of ww3_grid
   (for creating offline remaping files). This required adding netCDF support
   to ww3_grid

 - Also includes minor fix for the /O7a switch in w3iopomd.ftn

* modify the model/ftn/w3triamd.ftn and model/ftn/wmscrpmd.ftn and change from global grids from min(longitude)=-180, max(longitude)=180 to \delta(longitude)=360 degrees and added the regression test ww3_tp2.21

* bug fix for ww3_grid make without SCRIP switch

* Fix for corner node periodicity in wmscrpmd.ftn

* PDLIB/yowpdlibmain.ftn: fix to handle global meshes

* bug fix for SCRIPNC switch

* update info for ww3_tp2.21 for domain decomposition and PDLIB option

* fix for mww3_04 link with SCRIP and SCRIPNC switch

* add inputs to tar file, update model/bin/ww3_from_ftp.sh and remove inputs from ww3_tp2.21

* reduce the duration of ww3_tp2.21 for the sake of regtest time

* small editorial fixes

* fixes for SCRIPNC switch

* change date for ww3_ounf and ww3_ounp for ww3_tp2.21

* edit w3_make

Co-authored-by: lewis.sampson <[email protected]>
Co-authored-by: Juan Manuel Castillo Sanchez <[email protected]>
Co-authored-by: Chris Bunney <[email protected]>
Co-authored-by: Ali Abdolali <[email protected]>
Co-authored-by: Steven Brus <[email protected]>
Co-authored-by: Steven Brus <[email protected]>
Co-authored-by: Lorenzo Mentaschi <[email protected]>
ukmo-ccbunney added a commit that referenced this pull request Apr 1, 2021
* Added boundary checks to the SMC grid input files for ww3_grid,
to ensure they comply with the limits of the nameslist.

* Fb 360 calendar (#8)

Changes to add support to 360-day and 365-day (no leap year) calendar - see ticket NOAA-EMC#209
  * Additional CALTYPE namelist parameter in MISC section
  * New ww3_tc1 regtest.

* RTD support for ww3_boun[dc] (#10)

* Updated ww3_bound and ww3_bounc to handle model grids formulated on a rotated pole.
* Manual and nml/inp files to updated clarify that ww3_bound/ww3_bounc only accept input spectra formulated on a standard pole grid.

* Fb coupling time (#9)

Updates to allow a coupling time step that is different from the model time step. 
* Includes new regtest (in ww3_tp2.14) for non-default oasis time step.
* ww3_tp2.14 regtest added to matrix.base.

* bug fix for ukmet development

* add the matrix subsetter

* clean-up

* clean up

* add another script which separate serial and parallel jobs and divide them

* modify the script to remove ../model? after test completion.

* bug fixes and adding ww3_tp2.17 to list_heavy

* add if statement to remove matrix? and model?

* Update matrix_divider_p.sh

* Merge remote-tracking branch 'upstream/develop' into fb_matrix_divider

* Merge remote-tracking branch 'upstream/develop' into fb_matrix_divider

* put if check for ../model? inside matrix? loop

* fix the bug for sed for model?

* final fix for extra model? removal

* add 2.21 to the list_heavy

Co-authored-by: lewis.sampson <[email protected]>
Co-authored-by: Juan Manuel Castillo Sanchez <[email protected]>
Co-authored-by: Chris Bunney <[email protected]>
Co-authored-by: Ali Abdolali <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants