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 oasis t+0 #13

Merged
merged 18 commits into from
Oct 5, 2020
Merged

Fb oasis t+0 #13

merged 18 commits into from
Oct 5, 2020

Conversation

ukmo-juan-castillo
Copy link

Changes to couple by default with oasis at T+0. Sent coupling fields are written in the restart to allow proper restartability of coupled models. The fields needed for coupling restartability can be written to the output as an option given by a namelist parameter also in uncoupled configurations.

New regtests have been added to test all the new functionality, including running a coupled model without coupling lag. The tests already present in tp2.14 do not pass because now the first coupling exchange is done at the beginning of the run, rather than after the first time step.

Minor changes have been added to w3iosfmd.F90 to allow compilation with the switch MPIT (debugging code for MPI).

Other minor modification was added to make sure that the integer variable ID_OASIS_TIME was assigned the right value when made equal to a real value.

Details related to the inner workings of OASIS coupling should be properly documented when updating the coupling section of the manual.

For more details see issue 232 at NOA: NOAA-EMC#232

UKMO-lsampson and others added 8 commits July 22, 2020 11:44
to ensure they comply with the limits of the nameslist.
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.
* 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.
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.
@mickaelaccensi
Copy link
Collaborator

Hi Juan,
I would like to fully understand your changes in the code and take the time to run the regtests. I won't be able to do it until Thurday 3rd or Friday 4th. Sorry for the delay.

@ukmo-ccbunney
Copy link
Member

Adding @ukmo-ansaulter as a reviewer for this PR as I think he has a better insight into the coupling aspects of WW3 than I do.

@mickaelaccensi
Copy link
Collaborator

It's a really good work and it seems to work correctly on a quick test I've done with WW3 and an atmospheric model MESONH. By the way, I'm a bit worried to loose the original way it was done since many users are still using it with a lag and netcdf restart file, so could you explain if the restart file used to send the first field by OASIS (r-ww3.nc in regtest) can still be used or not anymore ? I will play with the new regtests next week before PR validation, I didn't had time to do it today..
Could you also clarify the purpose of the variable SBSED (which replace somewhat OASISED) and the switch SBS.

@ukmo-juan-castillo
Copy link
Author

Yes, the restart file used to send the first field by OASIS when running with a coupling lag still can be used. To recover the same behaviour as in the original code, the r-ww3.nc should simply contain zeroes for the first coupling time step (as now the first coupling time step is time zero instead of time zero + coupling time step).

The OASISED variable in the w3wavemd.ftn, so I removed. I introduced the SBSED variable because when coupling with OASIS coupling I would prefer that writing to the output is not imposed (it will only be written when FLOUT(1) is true), but on the other hand I did not want to change the behaviour when using the SBS switch.

as the warning is intended to flag invalid values.
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 @ukmo-juan-castillo , these changes look OK to me although I have suggested some extension to a few comments

To note:

  1. my approval here assumes you can address @mickaelaccensi 's comments - I haven't looked at these aspects in my review

  2. the %EXTRA% namelist is an alternative functionality to the /WRST switch used by @JessicaMeixner-NOAA to add wind fields to the restart file - longer term it might be worth integrating these two approaches so that they are consistent?

  3. you've noted that the (OASIS) coupling documentation in the manual needs updating, is there an existing ticket for this?

@ukmo-juan-castillo
Copy link
Author

I just cleaned up the code a bit by removing a few lines that were not necessary as their functionality was duplicated.

@mickaelaccensi
Copy link
Collaborator

Yes, the restart file used to send the first field by OASIS when running with a coupling lag still can be used. To recover the same behaviour as in the original code, the r-ww3.nc should simply contain zeroes for the first coupling time step (as now the first coupling time step is time zero instead of time zero + coupling time step).

Maybe I did something wrong but I'm not able to reproduce the same behavior as in the actual develop branch, even setting r-ww3.nc values at zeroes, there is still a shift in time in the values in the output files WW3_* and TOY_*. I've tried different things but it's never the same output. Could you please check with ww3_tp2.14 and confirm ?

@ukmo-juan-castillo
Copy link
Author

There are two details here, the OASIS LAG and the restart file that is used. I read with more detail the OASIS manual, and I copy the relevant information below:

"When a component calls a oasis put, the value of the lag is automatically added to the value of the
date argument and the “put” is actually performed when the sum date+lag is a coupling time; in the
target component, this “put” will match a oasis get for which the date argument is the same coupling
time. So the lag only shifts the time at which the data is sent but not the time at which the data is received.
When the lag is positive, a restart file must be available to initiate the coupling. For a field with positive
lag, the source component automatically reads the field in the restart file during the coupling initialization
phase (below the oasis enddef) and send the data to match the oasis get performed at time=0 in
the target component."

In the present regtests the lag is positive (and equal to the coupling time step), so each model will receive the data contained in the restart file at time zero. The first time oasis_put is called from the model will be in our case at t=LAG=coupling time step. With the modifications in this branch, this is equivalent to coupling without lag (as by default oasis_put is always called at t=0), and the results with the two approaches will only be the same if the models are putting the same data at the same time step. This implies that in this branch the first oasis_put should be sending the same data as what is contained in the restart files.

I am afraid that we could only reproduce the coupling regtests with this branch if we trick the runs somehow, but users should be able to run as they were doing until now just by choosing an appropriate LAG. You could consider this branch as one branch that removes the need to use an OASIS restart file to send data from one model at time zero, so that the actual values of the coupling fields at time zero in the model are sent. This is how all the forecast models I have seen work. In the case of WWIII we need to include the coupling fields in the model restart file to couple properly at time zero just because the source terms are not calculated at time zero. Another option would be to actually calculate the source terms at time zero as it is done in other models, but I consider this would be much more complicated to do due to the way WWIII is coded.

After these considerations, I would suggest removing the oasis LAG and the oasis restart files in all regtests (or if you believe it is still necessary to test oasis lags, just leave one test case).

@ukmo-ccbunney ukmo-ccbunney removed their request for review September 15, 2020 11:22
@ukmo-juan-castillo
Copy link
Author

There are two details here, the OASIS LAG and the restart file that is used. I read with more detail the OASIS manual, and I copy the relevant information below:

"When a component calls a oasis put, the value of the lag is automatically added to the value of the
date argument and the “put” is actually performed when the sum date+lag is a coupling time; in the
target component, this “put” will match a oasis get for which the date argument is the same coupling
time. So the lag only shifts the time at which the data is sent but not the time at which the data is received.
When the lag is positive, a restart file must be available to initiate the coupling. For a field with positive
lag, the source component automatically reads the field in the restart file during the coupling initialization
phase (below the oasis enddef) and send the data to match the oasis get performed at time=0 in
the target component."

In the present regtests the lag is positive (and equal to the coupling time step), so each model will receive the data contained in the restart file at time zero. The first time oasis_put is called from the model will be in our case at t=LAG=coupling time step. With the modifications in this branch, this is equivalent to coupling without lag (as by default oasis_put is always called at t=0), and the results with the two approaches will only be the same if the models are putting the same data at the same time step. This implies that in this branch the first oasis_put should be sending the same data as what is contained in the restart files.

I am afraid that we could only reproduce the coupling regtests with this branch if we trick the runs somehow, but users should be able to run as they were doing until now just by choosing an appropriate LAG. You could consider this branch as one branch that removes the need to use an OASIS restart file to send data from one model at time zero, so that the actual values of the coupling fields at time zero in the model are sent. This is how all the forecast models I have seen work. In the case of WWIII we need to include the coupling fields in the model restart file to couple properly at time zero just because the source terms are not calculated at time zero. Another option would be to actually calculate the source terms at time zero as it is done in other models, but I consider this would be much more complicated to do due to the way WWIII is coded.

After these considerations, I would suggest removing the oasis LAG and the oasis restart files in all regtests (or if you believe it is still necessary to test oasis lags, just leave one test case).

Is there any progress on this branch? Before continuing with more issues I would prefer to have this merged to the staging area, as the next issues build on this branch (adding new coupling fields for example).

I believe that with this branch we can reproduce old results by running without lag, always than the values of the coupling fields at T+0 for the wave and the toy models are the same as the values contained in the OASIS restart.

@mickaelaccensi
Copy link
Collaborator

Hi Juan,

I've discussed it with other colleagues working on OASIS coupling with WW3, CROCO, NEMO, WRF, MESONH and since the implementation of OASIS is done in the common way for all the models (netcdf restart file for first timestep and lag=dt), we would rather keep the actual steps of coupling fields exchange since it does not change the results.

actual steps with lag=dt

  • initialization : oasis_time = 0
  • rcv(oasis_time) (Note: at the beginning rcv(0) = get from oasis restart file)
  • timestepping: t=t+dt (Note: model run and increment the time the next time step)
  • snd(oasis_time) (Note: at the end of the first time step snd(0), OASIS will perform put(0+lag) = put (t+dt) )
  • oasis_time=oasis_time+dt (Note: oasis_time is updated for the next time step)

proposed steps with lag=0

  • initialization : oasis_time = 0
  • rcv(oasis_time) (Note: at the beginning rcv(0) = get from oasis restart file)
  • timestepping: t=t+dt (Note: model run and increment the time the next time step)
  • oasis_time=oasis_time+dt (Note: oasis_time is updated for the next time step)
  • snd(oasis_time) (Note: at the end of the first time step snd(0), OASIS will perform put(180+lag) = put (t+dt) )

What was your initial need to implement the OASIS exchanges differently ?

@ukmo-juan-castillo
Copy link
Author

Hi Juan,

I've discussed it with other colleagues working on OASIS coupling with WW3, CROCO, NEMO, WRF, MESONH and since the implementation of OASIS is done in the common way for all the models (netcdf restart file for first timestep and lag=dt), we would rather keep the actual steps of coupling fields exchange since it does not change the results.

actual steps with lag=dt

* initialization : oasis_time = 0

* rcv(oasis_time)                     (Note: at the beginning rcv(0) = get from oasis restart file)

* timestepping: t=t+dt              (Note: model run and increment the time the next time step)

* snd(oasis_time)                    (Note: at the end of the first time step snd(0), OASIS will perform put(0+lag) = put (t+dt) )

* oasis_time=oasis_time+dt    (Note: oasis_time is updated for the next time step)

proposed steps with lag=0

* initialization : oasis_time = 0

* rcv(oasis_time)                     (Note: at the beginning rcv(0) = get from oasis restart file)

* timestepping: t=t+dt              (Note: model run and increment the time the next time step)

* oasis_time=oasis_time+dt    (Note: oasis_time is updated for the next time step)

* snd(oasis_time)                    (Note: at the end of the first time step snd(0), OASIS will perform put(180+lag) = put (t+dt) )

What was your initial need to implement the OASIS exchanges differently ?

The proposed steps with lag=0 imply that, to do what we need at the Met Office, OASIS will have to write a restart file every time for each model involved in coupling, which can be done by calling the oasis_put routine with an optional parameter. This is not a standard practice in any of the models we worked with, and would require modifications to these codes and the working standards in the whole office.

My proposal then is, if you want to preserve the exact current behaviour, having another input parameter to decide if coupling will happen at time step zero, so that when it is set to false (this should be the default value) nothing is changed from the original behaviour of the code, and when it is true this branch implementation takes place. You can see this branch then as adding extra coupling flexibility to the code.

Is this acceptable, or do you have a different proposal?

@mickaelaccensi
Copy link
Collaborator

ok it sounds to be a good approach

@ukmo-juan-castillo
Copy link
Author

ok it sounds to be a good approach

I just committed the recommended changes, and they are ready for a new review. I am running all the regtests again just in case, I did it before but I had small differences because I was using old input data.

@ukmo-juan-castillo
Copy link
Author

ok it sounds to be a good approach

I just committed the recommended changes, and they are ready for a new review. I am running all the regtests again just in case, I did it before but I had small differences because I was using old input data.
I added a small correction. Some lines of code in ww3_shel had to be moved so that the variable OARST was properly updated, as it depends on some variables that are not updated until later in the code. This modification only affect oasis coupled runs and runs writing extra output with the new code in this branch (regtests ww3_tp2.14, which are already successfully tested).

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 @ukmo-juan-castillo - great job from you and @mickaelaccensi working this all through

Happy to approve this branch

@ukmo-ccbunney ukmo-ccbunney changed the base branch from develop to staging_oct20 October 5, 2020 08:41
@ukmo-juan-castillo
Copy link
Author

Merged the final version to the staging branch.

Final comment as a summary:

In the original code a coupling lag had to be used, coupling took place at the last time step but not at the first, and the coupling fields had to be written in an oasis restart file. With this ticket, the program can run in a different way by not using a coupling lag, coupling taking place at the first time step but not the last, and the coupling fields are written in the wave restart file - no extra oasis restart file needed.

@ukmo-ccbunney
Copy link
Member

@mickaelaccensi - are you happy with these latest changes?

@mickaelaccensi
Copy link
Collaborator

yep, I've tested it and it's ok.
could you just update the w3_clean script to fully clean regtest ww3_tp2.14
then I'll approve it

@ukmo-juan-castillo
Copy link
Author

yep, I've tested it and it's ok.
could you just update the w3_clean script to fully clean regtest ww3_tp2.14
then I'll approve it

I run the script and it removes everything, except the input files copied by ww3_from_ftp.sh (but the input files are not deleted from other tests either) and the grid_toy_model.nc file generated by the ww3_tp2.14 script Create_grid_and_restart_files_for_TOY.py (that was not deleted originally, I believe). Is anything in particular that is not deleted by the w3_clean script that you would like to delete?

@mickaelaccensi
Copy link
Collaborator

ok it was files from ww3_from_ftp.sh ! approved

Copy link
Collaborator

@mickaelaccensi mickaelaccensi left a comment

Choose a reason for hiding this comment

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

new version approved

@ukmo-ccbunney
Copy link
Member

ukmo-ccbunney commented Oct 5, 2020

ok it was files from ww3_from_ftp.sh ! approved

Many thanks for the thorough review, @mickaelaccensi. :)

@ukmo-ccbunney ukmo-ccbunney merged commit f103922 into staging_oct20 Oct 5, 2020
ukmo-ccbunney pushed a commit that referenced this pull request Oct 28, 2020
In the original code a coupling lag had to be used, coupling took place at the last time step but not at the first, and the coupling fields had to be written in an oasis restart file. With this ticket, the program can run in a different way by not using a coupling lag, coupling taking place at the first time step but not the last, and the coupling fields are written in the wave restart file - no extra oasis restart file needed.
ukmo-ccbunney added a commit that referenced this pull request Nov 5, 2020
* Fb oasis t+0 (#13)

In the original code a coupling lag had to be used, coupling took place at the last time step but not at the first, and the coupling fields had to be written in an oasis restart file. With this ticket, the program can run in a different way by not using a coupling lag, coupling taking place at the first time step but not the last, and the coupling fields are written in the wave restart file - no extra oasis restart file needed.

* Fb uprstr inp (#15)

Enhancement to improve the way in which input data is read in and logged by the ww3_uprstr program:
  * read in variables specific to the update process selected
  * output the values provided in the ww3_uprstr.out log file
  * update the .inp template file and regtests to improve clarity and work with the changes
  * add capability to read inputs from a namelist (ww3_uprstr.nml) file

* Bf unconforming where in coupling routines (#17)

* Fix non-conforming WHERE statements in coupled routines
* Stop comparing history lines in OASIS rmp files

* Changes for efficient SMC grid coupling (#16)

* Changes for efficient SMC grid coupling
* Ensure consistency between SMC coupled test nml and inp files

Co-authored-by: Juan Manuel Castillo Sanchez <48921434+ukmo-juan-castillo@users.noreply.github.com>
Co-authored-by: Andy Saulter <48921142+ukmo-ansaulter@users.noreply.github.com>
@ukmo-ccbunney ukmo-ccbunney deleted the fb_oasis_t+0 branch February 24, 2021 12:34
ukmo-ccbunney added a commit that referenced this pull request May 19, 2021
This PR adds new fields that can be sent (mean wave number, mean wave period, total ocean wind stress) and received (air density, wind stress) to/from other models via OASIS coupling, which will also be available in the model output. The fields that can be received via coupling can also be read from an external file, and using a homogeneous ice fraction is now activated when using '.inp' input files, as it was already possible to do it using '.nml' input files. In order to add this functionality it was necessary to modify the format of the ww3_shel.inp and ww3_multi.inp files, so that new flags controlling if these fields are used can be read. Furthermore, the format of the ww3_outf.inp file must be modified to accommodate the new output fields available.

If no air density is read from file or via coupling, the default constant value is used. The infrastructure to read and use the wind stress is already in place, but the actual use of wind stress to drive the model is not implemented yet and it will be the aim of issue NOAA-EMC#337

* Fb oasis t+0 (#13)

In the original code a coupling lag had to be used, coupling took place at the last time step but not at the first, and the coupling fields had to be written in an oasis restart file. With this ticket, the program can run in a different way by not using a coupling lag, coupling taking place at the first time step but not the last, and the coupling fields are written in the wave restart file - no extra oasis restart file needed.

* Fb uprstr inp (#15)

Enhancement to improve the way in which input data is read in and logged by the ww3_uprstr program:
  * read in variables specific to the update process selected
  * output the values provided in the ww3_uprstr.out log file
  * update the .inp template file and regtests to improve clarity and work with the changes
  * add capability to read inputs from a namelist (ww3_uprstr.nml) file

* First set of changes, still testing

* Some fixes

* Further changes mainly fixing vector boundaries

* Main changes

* Most changes already present, only need to write in output and restart

* Further changes - activate coupling

* Bug fixes to make the code compile

* Further changes to run regtests

* Minor fixes for output, manual, and switches

* Small fix to comments in model input

* Remove some lines that had to be removed but were overlooked

* Substitute ATTX and ATXX switches by WNTX and WNXX; add comments
regarding last modification date

* Change in file missed in latest commit

* Minor changes fixing version change

* Small fixes after reviewer's comments and to fix regtest

* Changes to pass regtests

* Addition of a new regtest and small fixes to ww3_prnc

* Time-interpolate air density when read from file

* Ensure that units are consistent

* Restore long name in metadata for new fieds, and update code version for
input files needed in regtest

* Update last modification date; corrections for new output field: WNMEAN
has now index (2,19) instead of (2,18)

* Fixes from Jessica Meixner; update date of last change in restart file

* Small fix to the format of the example input file ww3_multi.inp

* Some fixes after merging with latest version of develop

* Correct one version number

* Fix the header of the table printed in log.ww3, should not cause changes
in the output

* Minor fix in the ww3_shel.inp file of one of the regtests

* Update ww3_shel.inp

* Some fixes and improvements that were proposed during the review process

* Added comment to the new regtests implemented in ww3_tp2.15

Co-authored-by: Andy Saulter <48921142+ukmo-ansaulter@users.noreply.github.com>
Co-authored-by: ukmo-chris.bunney <christopher.bunney@metoffice.gov.uk>
Co-authored-by: Ali.Abdolali <37336972+aliabdolali@users.noreply.github.com>
Co-authored-by: aliabdolali <ali.abdolali@noaa.gov>
Co-authored-by: Chris Bunney <48915820+ukmo-ccbunney@users.noreply.github.com>
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.

5 participants