Last edited Timestamp?

For completion by the Sci/Tech/Code? reviewer

Reviewer: [ Steven Alderson (sga@…) ]

Ticket Details, Documentation and Code changes

Do you understand the area of code being altered and the reasoning why it is being altered?	In part
Do the proposed code changes correspond with the stated reason for the change?	Yes
Is the in-line documentation accurate and sufficient?	It is consistent with existing code
Do the code changes comply with NEMO coding standards?	Almost
Is the Ticket documented with sufficient detail for others to understand the impact of the change?	NO
Does any corresponding external documentation require updating?	YES
If yes, which docs and have the updates been drafted?	YES/NO
Are namelist changes required for this change?	YES/NO
If yes, have they been done?	YES/NO
Has a completed Ticket Summary template been appended to the ticket to aid code reviews	YES/NO
Does this summary correspond with your understanding of the full ticket?	YES/NO

Ticket, Documentation and Code comments

Add specific Ticket, Documentation and code comments here

Testing

Has the NVTK and other jobs been tested with this change?	YES/NO
Have the required bit comparability tests been run?	YES/NO
Can this change be shown to have a null impact? (if option not selected)	YES/NO
If no, is reason for the change valid/understood?	YES/NO/NA
If no, ensure that the ticket details the impact this change will have on model configurations .	YES/NO/NA
Is this change expected to preserve all diagnostics?	YES/NO
If no, is reason for the change valid/understood?	YES/NO/NA
Are there significant changes in run time/memory?	YES/NO

Testing Comments

I, the reviewer, have not yet managed to get this code to compile. Problems encountered so far:

the name of the C compiler (cc) is hard-coded into NEMOGCM/EXTERNAL/AGRIF/LIB/Makefile; one cpp command I'm using doesn't like a space between '-I' and the include directory name;

Obscure build errors occur on two different systems I've tried so far, so I will persevere. However, it seems a bit delicate and may therefore present support issues.

Add specific testing comments here

Add specific testing comments here

Code Review

Do the code changes comply with NEMO coding standards?	Almost
Are code changes consistent with the design of NEMO?	Yes
Is the code free of unwanted TABs?	unchecked
Has the code been wholly (100%) produced by NEMO developers working on NEMO?	YES/NO
If no, ensure collaboration agreement has been added to the ticket keywords

Add specific code comments or suggested alterations here.

Code differences between trunk revision 2803 and branch revision 2815:

NEMOGCM/NEMO/OPA_SRC/nemogcm.F90 NEMOGCM/NEMO/OPA_SRC/SBC/sbcice_lim_2.F90

childfreq is incremented inside a piece of code run only every nn_fsbc timesteps Does nn_fsbc have to be 1 for this to work?

NEMOGCM/NEMO/OPA_SRC/BDY/bdy_oce.F90 NEMOGCM/NEMO/OPA_SRC/BDY/bdytides.F90 NEMOGCM/NEMO/OPA_SRC/OBS/obs_sla_types.F90 NEMOGCM/NEMO/OPA_SRC/OBS/obs_types.F90

unless I've done my comparisons incorrectly, it's not clear to me why BDY and OBS changes have been made

NEMOGCM/NEMO/OPA_SRC/par_oce.F90 NEMOGCM/NEMO/LIM_SRC_2/limhdf_2.F90

changes here would seem to be just to avoid work array clashes so would appear to belong in the trunk rather than here

NEMOGCM/NEMO/LIM_SRC_2/limtrp_2.F90 NEMOGCM/NEMO/LIM_SRC_2/limadv_2.F90

more work space changes

NEMOGCM/NEMO/LIM_SRC_2/limrhg_2.F90 NEMOGCM/NEMO/LIM_SRC_2/iceini_2.F90 NEMOGCM/NEMO/LIM_SRC_2/ice_2.F90 NEMOGCM/NEMO/LIM_SRC_3/limrhg.F90

calls agrif_dyn_lim with cd_type of 'U' or 'V', but this routine (in agrif_lim2_interp.F90) does most work only when cd_type is 'V' is this correct?

NEMOGCM/NEMO/NST_SRC/agrif_user.F90

Agrif_InitValues has a commented out call to Agrif_InitValues_cont_lim2 it would seem best either to delete this or add comments as to when it would be required.

NEMOGCM/NEMO/NST_SRC/agrif_ice.F90 NEMOGCM/NEMO/NST_SRC/agrif_opa_update.F90 NEMOGCM/NEMO/NST_SRC/agrif2model.F90 NEMOGCM/NEMO/NST_SRC/agrif_lim2_interp.F90

code in here seems rather obscure and requires more comment. For example the treatments of u_ice_oe,v_ice_oe and u_ice_sn,v_ice_sn are both asymmetrical in agrif_dyn_lim. They are also declared with a 4 by 2 pattern. Why?

NEMOGCM/NEMO/NST_SRC/agrif_lim2_update.F90 NEMOGCM/NEMO/NST_SRC/agrif_oce.F90

Review Summary

I believe that this code has already been used by several groups with NEMO v3.2. There is an argument therefore for acceptance in v3.3.1 with these groups retesting as appropriate, assuming that the extra agrif code here is substantially unchanged from the extra code they were using.

Approval for the trunk

YES/NO

The code reviewer may approve the change for the NEMO trunk when:

1. their requests/comments have been addressed satisfactorily.
2. the above check-list has been completed.

or the code reviewer may choose to reject & assign the change back to the code author.