Last edited [[Timestamp]] [[PageOutline]] == For completion by the Sci/Tech/Code reviewer == '''Reviewer:''' [ Rachid Benshila ] Suggestion of cosmetic changes: '''1. namelist ''' - integer in namelist should be nn_something, ex niaufn=nn_iaufn[[BR]] - real in namelist should be rn_something, ex saltfixmin = rn_saltfixmin[[BR]] - character name (here mainly filename) should be cn_something, enactfiles=cn_enactfiles[[BR]] Presentation of the namelist changed since nemo_v3.1(?), it should be something like (see [https://forge.ipsl.jussieu.fr/nemo/browser/trunk/CONFIG/ORCA2_LIM/EXP00/namelist)[[BR https://forge.ipsl.jussieu.fr/nemo/browser/trunk/CONFIG/ORCA2_LIM/EXP00/namelist )] !-------------------------------[[BR]] &namobs ! observation usage switch[[BR]] !----------------------------------[[BR]] ln_t3d = .true. ! Logical switch for T profile observations [[BR]] ln_s3d = .true. ! Logical switch for S profile observations[[BR]] Usually, we report the namelist modifications for each standard configuration under CONFIG directory(except ORCA2_OFF_PISCES which is offline biogeochemistry), even when they are not in use and all parameters et to false (it's quite painful to do) '''2. Code ''' '''2.1 Cosmetic ''' * OPA_SRC/SBC : geo2ocean (obs_rot) REAL(wp), DIMENSION(jpi,jpj), INTENT( OUT ):: &[[BR]] & psinu, pcosu, psinv, pcosv! copy of data could be REAL(wp), DIMENSION(jpi,jpj), INTENT( OUT ):: psinu, pcosu, psinv, pcosv! copy of data * OPA_SRC/OBS  and OPA_SRC/ASM: [[BR]] header is missing in new routines (see existing routine), sometimes there are reference to NEMOVAR licence[[BR]] !!----------------------------------------------------------------------[[BR]] !! NEMO/OPA 3.3 , LOCEAN-IPSL (2010)[[BR]] !! $Id$[[BR]] !! Software governed by the CeCILL licence (modipsl/doc/NEMO_CeCILL.txt)[[BR]] !!----------------------------------------------------------------------[[BR]] * same remark as above for some declarations on several lines with &[[BR]] * module mpp_map could be include in OPA_SRC/lib_mpp.F90 since it's not specific to OBS[[BR]] * sentence "module used", "routine accessibility", "arguments local declaration" are obsolete (see attached routine as example)[[BR]] * majority of variables follows the DOCTOR norme, some does not see (http://www.nemo-ocean.eu/Media/Files/coding_rules_OPA9), especially fortran structures which should be svar for module variables and sdvar fro dummy arguments '''2.2 Code itself ''' * calendar functionalities : routines julian.F90 and related include files IOIPSL library provides similar calendar functionalities in calendar module (I'm not sure for the optional kdate argument), we are using it in the rest of the code: USE IOIPSL[[BR]] CALL ymds2ju (year,month,day,sec,julian)[[BR]] CALL ju2ymds (julian,year,month,day,sec)[[BR]] * in OBS, It may be logical to define kind of data fbsp and fbdp in par_kind.F90 rather than in obs_fbm[[BR]] * in OBS, Using nf90_open rather than iom_open may be a sources a problem when using nesting (maybe one day) with key agrif (I should talk with Arthur and Eric), Idon't know if there is a reason to it, please use iom_open when possible[[BR]] * in OBS some routine in obs_mpp.F90 seems duplicated in lib_mpp.F90 (obs_mpp_max_integer,obs_mpp_sum_integers in OBS and mpp_max, mpp_sum in lib_mpp) and anyway those which are not specific to obs should be included in lib_mpp.F90[[BR]] * in OBS module mpp_map could be include in OPA_SRC/lib_mpp.F90 since it's not specific to OBS[[BR]] * in ASM, routine calc_month_len does the same thing than ioget_mon_len provided by IOIPSL '''3. MISC ''' It would be nice to have access to an observation file (enact) or at least a reference to a link to one in the routine to use key_diaobs '''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?||YES|| ||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?||YES|| ||Does any corresponding external documentation require updating?||YES|| ||If yes, which docs and have the updates been drafted?||NO|| ||Are namelist changes required for this change?||YES|| ||If yes, have they been done?||NO|| ||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 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|| ||Can this change be shown to have a null impact? (if option not selected)||YES|| ||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|| ||If no, is reason for the change valid/understood?||YES/NO/NA|| ||Are there significant changes in run time/memory?||YES/NO|| Testing Comments Add specific testing comments here Add specific testing comments here === Code Review === ||Do the code changes comply with NEMO coding standards?||YES/NO|| ||Are code changes consistent with the design of NEMO?||YES/NO|| ||Is the code free of unwanted TABs?||YES/NO|| ||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. === Review Summary === Add summary here === 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. 1. the above check-list has been completed. or the code reviewer may choose to reject & assign the change back to the code author.