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

Coupling frequency overridden to time step #229

Closed
ukmo-juan-castillo opened this issue Jul 8, 2020 · 11 comments
Closed

Coupling frequency overridden to time step #229

ukmo-juan-castillo opened this issue Jul 8, 2020 · 11 comments
Assignees
Labels
enhancement New feature or request

Comments

@ukmo-juan-castillo
Copy link
Collaborator

In the ww3_shel.F90 code, there are some lines that explicitly force the coupling time step to be equal to the model time step, even when the coupling time step is read from the namelist:

!/COU ! Force coupling time step to model time step
!/COU IF ( J .EQ. 7 .AND. DTTST.NE.DTMAX ) THEN
!/COU IF ( IAPROC .EQ. NAPOUT ) THEN
!/COU WRITE (NDSO,1009) DTTST, DTMAX
!/COU END IF
!/COU ODAT(5*(J-1)+3) = INT(DTMAX)
!/COU DTTST = DTMAX
!/COU END IF

What it seems it happened is that coupling was first implemented using a fixed value of the coupling time step, and in a later stage the option of reading the coupling time step from the namelist was implemented, but without actually using the read value.

This behaviour is not described in the manual and can be very confusing to unaware users, as if they specify a coupling time step different to the model time step, their runs will crash with an error very similar to an OASIS deadlock. Furthermore, the code in its present condition is quite inflexible, and it would be convenient to have the capacity of using any coupling time step, the only possible limitation being that the coupling time step should be divisible by the model time step.

There are different possible solutions to add this extra capability:

  • The simplest solution consists in just removing the previously described lines in the ww3_shel.F90 code. I already tested this using a coupling time step of one hour and a model time step of ten minutes and it works. As an extra check, the coupling frequency can be set to zero by default, and if it is required but not set in the namelist, throw and error and stop the program.

  • Another option would be setting the coupling time step to zero by default, and if it is not changed by reading it from the namelist, set it by default to the model time step as it is done now after throwing a warning message.

In both cases there is the option of checking for negative values of the coupling time step, and rejecting coupling time steps that are not divisible by the model time step.

@ukmo-juan-castillo
Copy link
Collaborator Author

This issue is related to issue #207 and #206 and could be implemented as part of them

@ukmo-ansaulter
Copy link
Collaborator

@aliabdolali , @JessicaMeixner-NOAA , @mickaelaccensi and @tjcnrl

I think this issue is most likely to impact code/models you have been working with. Do you have a preferred behaviour from the two options @ukmo-juan-castillo has suggested??

@mickaelaccensi
Copy link
Collaborator

ok to remove these lines and add a test on the value of the coupling time step to be sure it is divisible by the model time step.
it's better to have an error message and stop the program if the coupling time step is set to 0 or incorrectly set.
the regtest ww3_tp2.14 need to be update to confirm the correct behavior of another coupling time step than the global model time step.

@JessicaMeixner-NOAA
Copy link
Collaborator

For ESMF coupling, we use wmesmf and the multi routines. We do not use the COU switch, so whatever works for @mickaelaccensi is fine with me.

@aliabdolali
Copy link
Contributor

I am fine with removing the lines, check the default time step (0) and defined one in the namelist and an error message if it is not divisible by the model time step, update of manual, and a regtest (ww3_tp2.14).
@ukmo-juan-castillo, @mickaelaccensi, could you take care of it?

@ukmo-juan-castillo
Copy link
Collaborator Author

I can take care of it. Should I create a branch in NOAA-EMC directly or in the ukmo-waves fork (or even reuse one of the branches planned for taking care of #207 or #206 )?

@JessicaMeixner-NOAA
Copy link
Collaborator

There should not be any development branches in the NOAA-EMC fork. So your personal fork or the ukmo-waves fork depending on the ukmo-waves fork's rules on creation of development branches in that fork.

@ukmo-ccbunney
Copy link
Collaborator

ukmo-ccbunney commented Jul 8, 2020

Should I create a branch in NOAA-EMC directly or in the ukmo-waves fork?

@ukmo-juan-castillo : I think it would be best to make this change in the UKMO fork. I feel like it could be incorporated into your coupling branch? Just link this issue into the PR when you raise one.

@ukmo-juan-castillo
Copy link
Collaborator Author

Given that we have a ticket for this change, I think it will be better if we have a separate branch in the UKMO fork. This will also make things easier for development of tickets #207 and #206 once it is merged.

@aliabdolali
Copy link
Contributor

Hi @ukmo-juan-castillo
You are right, please follow the guideline (https://github.com/NOAA-EMC/WW3/wiki/Developer-Guide). You need to create a feature branch, addressing this fix and send the PR to UKMET repo. After passing the regression tests at UKMET, Andy/Chris will send a PR to NOAA repo.

@ukmo-juan-castillo
Copy link
Collaborator Author

This improvement has already been successfully implemented, tested, and merged to the staging branch of the ukmo-waves /
WW3 repository.

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

No branches or pull requests

6 participants