Skip to content

Fb rtd2#7

Merged
ukmo-ccbunney merged 11 commits into
ukmo-waves:stagingfrom
CarstenHansen:fb_rtd2
Sep 15, 2020
Merged

Fb rtd2#7
ukmo-ccbunney merged 11 commits into
ukmo-waves:stagingfrom
CarstenHansen:fb_rtd2

Conversation

@CarstenHansen
Copy link
Copy Markdown
Collaborator

This feature allows the user to specify output boundary conditions to rotated grids under switch /RTD.

Explanations are added to the manual Sect. "3.4.9 Rotated grids".

The pole of each destination grid is specified as the I'th element
of arrays BPLAT(1:9) and BPLON(1:9) in a namelist &ROTB, e.g. as
'&ROTB BPLAT(2) = 20., BPLON(2) = -130., / ! boundary conditions to nest2.ww3'

The default of the namelist &ROTB is output to standard spherical grids,
BPLAT(1:9)=90, BPLON(1:9)=-180.

@ukmo-ccbunney
Copy link
Copy Markdown
Member

Hi @CarstenHansen. It looks like your branch has some merge conflicts with our develop branch. Is your branch up-to-date with the latest development from the NOAA develop branch? Looking at the commit history in this branch, I cannot see the latest commits that are present in the NOAA/UKMO develop branches...

@CarstenHansen
Copy link
Copy Markdown
Collaborator Author

Hi @ukmo-ccbunney.

When I press the '+- Compare' link on the github webpage and choose 'base repository: ukmo-waves/WW3, base: develop', it says:
'Able to merge. These branches can be automatically merged.'

I see that I added the latest commit 8e7e264 to my branch on May 28, which was a small, trivial update. This commit has 2 parents 239b75f + 3085d9f. 3085d9f is the present NOAA-EMC develop, and 239b75f was my previous merge of NOAA-EMC develop. Would it help to revert this commit 8e7e264 to 3085d9f ?

I wonder why I cannot view change to fb_rtd2 in commit 8e7e264 on the github web page. I think it may be because every time I do a merge of NOAA-EMC develop, it automatically creates a new commit.

This feature requires the switch RTD. It allows the user to specify
the pole of each nested grid for output of boundary conditions ('b.c.')
directly to files nestI.ww3. The pole is specified as the I'th element
of arrays BPLAT(1:9)/BPLON(1:9) in a namelist &ROTB. For e.g. I=2, a
nested grid may be rotated with pole specified as
'&ROTB BPLAT(2) = 20., BPLON(2) = -130., / ! b.c. to nest2.ww3'

The default of &ROTB is output to standard spherical grids,
BPLAT(1:9)=90, BPLON(1:9)=-180. With a value of BPLAT(I)==90, it is
required that BPLON(I)=-180.

In the forecast production at FCOO we run ww3_shel for a sequence of
nested models where some (Arctic and Greenland) have rotated lat/lon
coordinates, and others (Atlantic, European North-West Shelf and
the Baltic) have standard lat/lon coordinates.

Output b.c. to standard lat/lon is fairly simple even from a rotated
model. Without the present feature you may output point spectra at
the precise boundary points and post-process the output using
ww3_bounc or ww3_bound to generate nest.ww3 for the nested model. But
if output b.c. are to a rotated grid you would have to remap each point
to standard lat/lon values before writing the ww3_grid.{inp,nml} file.

Using the present feature a remapping of boundary conditions from
rotated lat/lon coordinates to standard lat/lon values is performed
internally in the ww3_grid program. These standard lat/lon values are
stored in the mod_def file and will be written to nestI.ww3 during
model runs.

Effects of this feature:

- There is no effect unless the switch RTD is set for compilation.

- The contents of b.c. output files nestI.ww3 are not affected.

- A model compiled with combined switches SMC and RTD is not affected.

- The default grid pole is the geographic north of a standard lat/lon
  grid. This corresponds to a &ROTD namelist with values
  '&ROTD PLAT==90., PLON=-180., UNROT=.FALSE. /' (these are the only
  allowed values with PLAT==90). This default alleviates output b.c. to
  a rotated grid from a standard lat/lon grid.

- Modified program ww3_grid and subroutine W3IOBC: The output b.c.
  coordinates are remapped to standard lat/lon and stored in the
  mod_def file. This remapping is not performed during runtime when
  writing to nestI.ww3. When PLAT==90, the array AnglD in the mod_def
  file is zero-valued.

- Modified subroutine W3UBPT: Rotation of spectra is performed only if
  PLAT<90.

- Modified program ww3_ounf: With PLAT==90 (if the grid is not
  remapped from an SMC grid), the NetCDF field output has no attributes
  related to a rotated pole grid mapping and does not contain the 2D
  variables standard_latitude and standard_longitude.

- Modified input examples inp/ww3_grid.inp, nml/ww3_grid.nml, and added
  nml/namelists_GULF.nml.
@aliabdolali
Copy link
Copy Markdown
Collaborator

Hi @CarstenHansen
Do we have a regression test for this feature? It's a necessity to have one to check this development and make sure it won't be affected by future developments.
@ukmo-ccbunney I think this is a milestone, what do you think?

@CarstenHansen
Copy link
Copy Markdown
Collaborator Author

Hi @aliabdolali. I forgot to link to the issue NOAA-EMC#68 in which I originally described this enhancement. I understood very little about regtests at that time, and @ukmo-ccbunney and @ukmo-ansaulter offered to help performing the regtests - maybe incorporate it in the regtest for RTD.

@aliabdolali
Copy link
Copy Markdown
Collaborator

Hi @CarstenHansen
I made a milestone for this development on NOAA repo.
https://github.com/NOAA-EMC/WW3/milestone/24
Please let me know if you need help with regtests preparation for RTD. I'd suggest looking at one of the regression test structure and prepare your input accordingly + info. Info contains the test description. If you have a netcdf or a large file for the input, send it to me, so I can add it to our ftp.
We put some guides for regtest preparation on our wiki page.
https://github.com/NOAA-EMC/WW3/wiki/Checklist-for-Adding-or-Modifying-a-Regtest

@CarstenHansen
Copy link
Copy Markdown
Collaborator Author

Hi @aliabdolali
Thank you for your advice. I think we should await the reviews by @ukmo-ccbunney and @ukmo-ansaulter. They may also come out with an example of an SMC grid with boundary conditions to a rotated lat-lon grid. I will be on vacation for two weeks from today, and send you my input files thereafter.

Copy link
Copy Markdown

@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 for doing such a great job @CarstenHansen

I've tested these changes with our standard grid global model -> rotated grid regional model set-up and everything works OK.

In addition to the comments I've added here one further suggested change would be to up the precision of the error outputs associated with BCs; one issue I did have was in diagnosing where I'd set the BCs up incorrectly onto a high resolution grid. The suggested change is for lines 6051-52

994 FORMAT ( ' *** POINT OUTSIDE GRID (SKIPPED) : X,Y =',2F10.5)
995 FORMAT ( ' *** POINT ON LAND (SKIPPED) : X,Y =',2F10.5)

Regarding the regtest, @ukmo-ccbunney need to have a look at what to do and whether the existing tp2.11 is fit for purpose. So, if you are happy with the proposed changes, suggest you mgiht deal with these now whilst we come up with a plan for the regtest.

Comment thread model/ftn/w3iobcmd.ftn Outdated
Comment thread model/ftn/w3updtmd.ftn Outdated
Comment thread model/ftn/w3updtmd.ftn Outdated
Comment thread model/ftn/ww3_grid.ftn Outdated
Comment thread model/nml/namelists.nml
Comment thread manual/num/rotagrid.tex Outdated
@CarstenHansen
Copy link
Copy Markdown
Collaborator Author

Great to see that your tests were OK @ukmo-ansaulter.
I will adopt your proposed changes. Regarding the need for a high resolution of the error message for points outside the grid or on land, I think that if anybody uses Cartesian coordinates (Easting/Northing), they should see the error output in units of m. The format descriptors 2994, 2995 are there and are brought into use with the construction:

            IF ( FLAGLL ) THEN
                WRITE (NDSE,2994) FACTOR*XO, FACTOR*YO
              ELSE
                WRITE (NDSE,994) FACTOR*XO, FACTOR*YO
              END IF

…tion of error message, full template text in namelists.nml, clearify the manual
Copy link
Copy Markdown

@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 for adopting these changes @CarstenHansen

In terms of the code, I approve this review. Will finalise an approval once @ukmo-ccbunney has revised and tested the new RTD regtests

@ukmo-ccbunney
Copy link
Copy Markdown
Member

@CarstenHansen I have some regression tests ready for this change; we need to get them added to your branch. You can either:
a) Give me write access to your branch on GitHub and I will push the changes up, or:
b) I can put the changes on a separate branch in this repo and you can merge them in.
Let me know what you prefer.

@CarstenHansen
Copy link
Copy Markdown
Collaborator Author

a) Give me write access to your branch on GitHub and I will push the changes up, or:

@ukmo-ccbunney I have invited you as a collaborator, which is the way i seems you will have write (push) access.

@ukmo-ccbunney
Copy link
Copy Markdown
Member

ukmo-ccbunney commented Jul 29, 2020

I have invited you as a collaborator

Thanks @CarstenHansen - I've pushed a commit to your branch containing the regtests (see ww3_tr1).

There are 3 tests in seperate input directories (see the info file for more details). Basically, the first two tests (input, input_std) test the generation of output boundary conditions on target grids formulated on different poles. The difference between the tests is that for the first the source grid is formulated on a rotated pole, and the second is on a standard pole.

The third test under the input_bndin technically does not test your RTD2 changes - it tests the correct ingestion of input nest.ww3 files on to a rotated pole model. I have included it in this regtest set as it complements the other two tests. Technically, it requires the update to the ww3_bound program that is handled in a separate PR #10, although it should still run without errors (it just gets the distance calculations to the points wrong).

@CarstenHansen
Copy link
Copy Markdown
Collaborator Author

Thank you. Your explanation (and the file regtests/ww3_tr1/info) makes good sense to me.

@ukmo-ansaulter ukmo-ansaulter self-requested a review July 29, 2020 16:12
Copy link
Copy Markdown

@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.

Approving this request - the new regtests are appropriate for covering RTD functionality

Also updated version numbers for 7.11 RTDB milestone.
@ukmo-ccbunney
Copy link
Copy Markdown
Member

@CarstenHansen : I've just pushed up commit 9e24969 which contains a merge of the develop branch. There were a few changes required to fix some merge conflicts. I have also updated the version numbers inline with the milestone for this change.

Can you just check you are happy with the modifications?

@CarstenHansen
Copy link
Copy Markdown
Collaborator Author

Hi @ukmo-ccbunney. Commit 9e24969 looks fine, I just found one typo in model/ftn/w3_bound.ftn: 'version 7.06' should be 'version 7.11' ?

@ukmo-ccbunney
Copy link
Copy Markdown
Member

Hi @ukmo-ccbunney. Commit 9e24969 looks fine, I just found one typo in model/ftn/w3_bound.ftn: 'version 7.06' should be 'version 7.11' ?

Hi @CarstenHansen - I originally did not consider the ww3_bound/ww3_bounc changes to be part of the 7.11 milestone as they were more bugfixes to the original RTD functionality. However, it seems that the ww3_bounc.ftn module has been updated to 7.11, so let's keep the ww3_bound.ftn module the same and make it 7.11 too. I'll push up a modification. Thanks.

@ukmo-ccbunney
Copy link
Copy Markdown
Member

Regtest for ww3_tp2.11 (rotated pole regression test) shows no differences, except for the expected extra screen output from ww3_grid:

***
/home/d02/frey/WW3/CarstenRTD/regtests/output/ww3_tp2.11/work_SHRD_RTD/ww3_grid.out_diff.txt
***
146,148d145
<   &ROTD PLAT = 37.50, PLON = 177.50, UNROT =  F /
<   &ROTB BPLAT =  90.0,  90.0,  90.0,  90.0,  90.0,  90.0,  90.0,  90.0,  90.0,
<         BPLON =-180.0,-180.0,-180.0,-180.0,-180.0,-180.0,-180.0,-180.0,-180.0, /

I am going to run the full regression test suite now.

@CarstenHansen CarstenHansen changed the base branch from develop to staging August 26, 2020 11:55
@ukmo-ccbunney
Copy link
Copy Markdown
Member

ukmo-ccbunney commented Sep 2, 2020

Results of full regression test matrix: matrixComp.zip

Summary of differences:

  • ww3_tp2.11 : Expected differences in screen output of ww3_grid related to this PR (now shows new ROTB namelist defaults)
  • mww3_test_03: Existing issue: known to be not b4b.
  • ww3_tp2.18/work_TIDE_MPI : Existing issue: known differences in screen output of ww3_grid.
  • ww3_ta1 and ww3_ts1 : Existing issue: known differences in the ST4 tests - see issue ST4 b4b reproduciblity issue NOAA-EMC/WW3#249

I'm happy for this change to be merged.

@aliabdolali - are you happy with the changes?

Copy link
Copy Markdown
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.

All looks good to me now.

@ukmo-ccbunney
Copy link
Copy Markdown
Member

Just one final small commit - I forgot to add the ww3_tr1 test to the the all section of matrix.comp.
This does not affect the regtest results above as it is a new regtest directory created in this PR.

Copy link
Copy Markdown
Collaborator

@aliabdolali aliabdolali left a comment

Choose a reason for hiding this comment

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

It looks ok to me. Thanks for doing this clean work.

@aliabdolali
Copy link
Copy Markdown
Collaborator

I wanted to press approve bottom but pressed the comment bottom instead :) Please consider it approved.

@ukmo-ccbunney ukmo-ccbunney merged commit e81b80a into ukmo-waves:staging Sep 15, 2020
ukmo-ccbunney added a commit that referenced this pull request Sep 28, 2020
* Fb rtd2 (#7)

FB_rtd2: Output boundary conditions to rotated grids

* This feature allows the user to specify the pole of each nested grid output of boundary conditions (nest) file.
  - There is no effect unless the switch RTD is set for compilation.
  - The contents of b.c. output files nestI.ww3 are not affected.
  - A model compiled with combined switches SMC and RTD is not affected.

* Updated manual Sec. "3.4.9 Rotated grids"

* Added regtests for testing input/output BCs in rotated pole context.

* Updated revision of ww3_bound.ftn to 7.11

* Added new ww3_tr1 regtest (rotated pole) to matrix.base. Also, split the SMC/RTD regtests into their own separate switches.

Co-authored-by: ukmo-chris.bunney <christopher.bunney@metoffice.gov.uk>

* Double allocation error of FIELD variable [w3wavemd] (#14)

* Fixed allocation of FIELD variable.
* Removed defunct OMPX switches and FLOMP variable

* Change coupling condition so that it works with all compilers (#12)

Co-authored-by: Carsten Hansen <cha@fcoo.dk>
Co-authored-by: Juan Manuel Castillo Sanchez <48921434+ukmo-juan-castillo@users.noreply.github.com>
@CarstenHansen CarstenHansen deleted the fb_rtd2 branch January 19, 2021 08:18
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