New URL for NEMO forge!   http://forge.nemo-ocean.eu

Since March 2022 along with NEMO 4.2 release, the code development moved to a self-hosted GitLab.
This present forge is now archived and remained online for history.
ticket/0866/Review – NEMO
wiki:ticket/0866/Review

Version 1 (modified by JohnSiddorn, 12 years ago) (diff)

--

Last edited Timestamp?

For completion by the Sci/Tech/Code? reviewer

Reviewer: John Siddorn

Ticket Details, Documentation and Code changes

Do you understand the area of code being altered and the reasoning why it is being altered?YES
Do the proposed code changes correspond with the stated reason for the change?YES
Is the in-line documentation accurate and sufficient?IN-LINE COMMENTS SHOULD BE IMPROVED TO AID READABILITY
Do the code changes comply with NEMO coding standards?COULD BE IMPROVED -COMMENTS BELOW
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?NEMO BOOK HAS BEEN UPDATED - NOT BEEN SEEN BY REVIEWER, AND EXPECTED TO BE BASIC
Are namelist changes required for this change?YES
If yes, have they been done?YES
Has a completed Ticket Summary template been appended to the ticket to aid code reviewsNO
Does this summary correspond with your understanding of the full ticket?N/A

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
Have the required bit comparability tests been run?YES
Can this change be shown to have a null impact? (if option not selected)YES
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?YES
If no, is reason for the change valid/understood?NA
Are there significant changes in run time/memory?YES

Testing Comments

The code has been seemingly well tested by the author in a local configuration, and I am confident the code will work well in that context. The null tests have been performed using SETTE so the risk of impact on NEMO users who will not use the HPG code is very small. There is a risk that the code is not well tested yet in the trunk version of NEMO. This needs to be mitigated by testing at the beta stage by consortium partners (NOCL and/or MetO).

Add specific testing comments here

Add specific testing comments here

Code Review

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

There has been no time to do an in-depth review of the code, but the following are noticeable from a quick code reading

1) Code uses global arrays rhd and fse3w as local arrays, temporarily stored in local arrays. This is dangerous, and local arrays only should be updated in the code. This is an important point and should be changed before the merge if possible. 2) The use of many v similar loops with subtle differences to account for the direction of action is hard to follow and makes bugs easy to put in and hard to trace. A more structured approach with one subroutine that is passed information on the direction of action would allow the code to be written only once rather than repeated many times. This has been recommended by the reviewer in the past and not implemented due to a lack of time. Although the code has been made more readable since that recommendation was made, it is still not friendly for other developers or users and it is strongly recommended that this is updated in future releases. 3) Overall readability of the code is poor, irrespective of point 2 above, and it would and does take other developers a lot of time to understand what the code is doing. It is strongly recommended that in future releases some effort is made to improve code readability (pt 2 above may sole that problem). 4) Do loops (and other logical statements, subroutine labels etc) mainly use upper case, but sometimes a mix of upper case and lower case. This is not critical, but for readability all UPPER case should be used. 5) Comments such as " !how to add vector opt.? N.B., jpi&jpi rather than jpim1&jpjm1 are needed here " should be removed or acted upon. Not critical. 6) Functions such as integ3 have inaccurate and/or non-existent header information.

Review Summary

The code is an essential part of the coastal functionality of NEMO and so should be included in the 3.4 release given it is unlikely to impact non-coastal users. However, the short period to review the code and the only partial adherence to NEMO standards and general good coding practice need to be addressed in the future,

Approval for the trunk

YES, as long as the updating of global arrays rhd and fse3w is removed.

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.