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