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? | Unknown |
| Are namelist changes required for this change? | NO |
| If yes, have they been done? | NA |
| Has a completed Ticket Summary template been appended to the ticket to aid code reviews | I don't know what this is |
| 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? | Unknown |
| Have the required bit comparability tests been run? | Unknown |
| Can this change be shown to have a null impact? (if option not selected) | Unknown |
| If no, is reason for the change valid/understood? | NA |
| If no, ensure that the ticket details the impact this change will have on model configurations . | NA |
| Is this change expected to preserve all diagnostics? | NO |
| If no, is reason for the change valid/understood? | Yes, if related to ice regions |
| Are there significant changes in run time/memory? | Unknown |
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.
I believe that some groups are running the code in v3.2. One possibility would be to upgrade this branch to v3.4 at the merge, continue the development over until next year and then ask the same groups to test it for inclusion in the v3.5 release.
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? | Unknown |
| 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
why the additional use statements for agrif: there seem to be no other changes in the body of the routine
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
not really a comment for this branch, but the use of 42 separate arrays by LIM2 for properties and their moments seems a little clumsy, particularly since they only ever seem to be used together in groups of 6. I would suggest that since they are all module variables and therefore allocated with their own dedicated routine that they could be rewritten as (jpi,jpj,6) arrays. This would reduce the possibility of typing errors in the use of 42 very similar variable names, which has had to be added to agrif_lim2_interp.F90 below.
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. Maybe this is dealt with by the agrif preprocessor but how does routine agrif_declare_var know about tn_id,etc when it is declared in agrif_oce but there is no "USE" statement for this?
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? There are also routine names in french (and I have nothing against this!), which would seem to be against the coding rules. Also see comments about limtrp_2.F90 above.
NEMOGCM/NEMO/NST_SRC/agrif_lim2_update.F90 NEMOGCM/NEMO/NST_SRC/agrif_oce.F90
Suggestions:
I think that since the code is currently limited to LIM2 usage that sbcmod.F90 should be modified to stop the code or at least warn the user when LIM3 is selected with AGRIF
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.4 with these groups retesting as appropriate, assuming that the extra agrif code here is substantially unchanged from the extra code they were using.
However, documentation is a big issue for AGRIF (not just for this branch). I (the reviewer) had a go at adding an AGRIF section to the NEMO book, but failed since I could not understand the derivation of the boundary conditions implemented in agrif_opa_interp.F90 for example. The creator of the current code (Frederic Dupont?) obviously understood more than I. But we are building "a house of cards", where we add more code but no documentation to describe it.
I would suggest that:
- this branch is not included in the merge
- after NEMO v3.4 has been created at the merge party, this branch be updated to NEMO v3.4, but that it is carried over as a separate branch to next year;
- those who have been using the version of this code for v3.2, be asked to test this 3.4 branch
- that a completely new development branch is opened for next year which is solely for the creation of AGRIF documentation;
- that a group of AGRIF users/developers is assembled (eg Frederic Dupont, Rachid Benshila, Jean Marc Molines, etc) to jointly fill in the text;
- and that no other changes to AGRIF code are accepted until this documentation branch is complete.
Approval for the trunk
NO
The code reviewer may approve the change for the NEMO trunk when:
- their requests/comments have been addressed satisfactorily.
- the above check-list has been completed.
or the code reviewer may choose to reject & assign the change back to the code author.
