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/0859/Review – NEMO
wiki:ticket/0859/Review

Version 2 (modified by acc, 12 years ago) (diff)

--

Last edited Timestamp?

For completion by the Sci/Tech/Code? reviewer

Reviewer: Andrew Coward, NOCS

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? NO
Do the code changes comply with NEMO coding standards? YES
Is the Ticket documented with sufficient detail for others to understand the impact of the change? NO
Does any corresponding external documentation require updating? NO
If yes, which docs and have the updates been drafted? N/A
Are namelist changes required for this change? YES
If yes, have they been done? Not completely
Has a completed Ticket Summary template been appended to the ticket to aid code reviews YES
Does this summary correspond with your understanding of the full ticket? YES

Ticket, Documentation and Code comments

Generally the code changes have been well made in agreement with the NEMO coding standards and a useful functionality is added by these changes. The few coding problems identified in this first review can be easily corrected and the additional documentation requested should not be too great an obstacle to finishing this development fully.

Specifics:

namelist

changes are largely in place but at least one new variable is incorrectly named: ln_ascii exists in the namelists but exists as ln_flo_ascii in the code. This discrepancy calls into question the level of testing since the presence of an undeclared namelist variable should cause a runtime error. Probably last minute style changes were made after testing?

iodef.xml changes have been made but only to the GYRE and ORCA2_LIM configurations. Changes should be made in all cases.

nemogcm.F90. The only change has been to move flo_init before iom_init in the calling order. If this change is necessary then the reason needs to be stated (on the ticket?). If it isn't necessary then the change should be reverted

iom.F90. Minor readability issue: the #if/#endif pair around line 118 should be indented ("# if/# endif")

in_out_manager.F90: minor: preference would be for numflo rather than numfl as a unit number (to maintain the usual 6 character standard). If made this change needs to be followed through in flowri.F90 where numfl is used

flowri.F90:

blabla... comments at the top needs to be replaced with something meaningful zwork is an automatic array of length jpnfl*jpk which is never used. For consideration: The else part of the IF statement at line 140:

ELSE ! the float is not inside of current proc's area

is not necessary since the 6 arrays zero'd in the block were initialised outside of the loop

flodom.F90

Observation: We need a new code convention for namelist variables that have replaced parameters. jpnfl, jpnewflo and jpnrstflo are not parameters and therefore shouldn't start with jp. However, the same can be said of jpni, jpnj jpnij etc. so this is not the time to impose a change. To be discussed at the developer's meeting?.

naming convention for contained subroutines? Should they all start with flo_?: flo_add_new_floats, flo_add_new_ariane_floats, flo_findmesh, flo_dstnce

Policy on allocatable vs automatic?. add_new_floats assumes automatic arrays: iimfl, ijmfl, ikmfl etc. should these be allocatable, allocated on first call and saved?.

constants should have the _wp precision appended (e.g. constants in dstnce)

minor: # else at line 446 needs aligning correctly

flo4rk.F90 minor: No check on the return code from the allocate statement.

florst.F90

automatic vs allocatable again. iproc is automatic. iproc is also a bad choice of name (used in lib_mpp and OBS to hold processor number). Perhaps iperproc would be better? blabla.. comments at the top need to be replaced with something meaningful

CHAP_DIA.tex

Finally, the documentation is inadequate. Several of the new namelist variables are not described and this is a missed opportunity to expand this section beyond the original, placeholder paragraph. The relationship between jpnfl and jpnewflo is not explained and the logic of ln_ascii (or rather ln_flo_ascii) appears to be inverted. One would expect ln_flo_ascii (T) to provide ascii output and (F) to be the new (default) netcdf option. Check comments and use of ln_flo_ascii in documentation and code.

Code Review

Do the code changes comply with NEMO coding standards? YES
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

Add specific code comments or suggested alterations here.

Review Summary

Close to completion subject to the minor corrections requested here.

Puzzled by the No answer to "Is this change expected to preserve answers in all possible model configurations?". Why should the presence or otherwise of passive floats affect the answers?

Approval for the trunk

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.