Changes between Version 2 and Version 3 of ticket/1468/Review


Ignore:
Timestamp:
2015-10-05T09:48:16+02:00 (5 years ago)
Author:
bouttier
Comment:

Legend:

Unmodified
Added
Removed
Modified
  • ticket/1468/Review

    v2 v3  
    44 
    55== For completion by the Sci/Tech/Code reviewer == 
    6 '''Reviewer:''' [ Enter your name here ] 
     6'''Reviewer 1:''' Pierre-Antoine Bouttier 
    77 
    88=== Ticket Details, Documentation and Code changes === 
    9 ||Do you understand the area of code being altered and the reasoning why it is being altered?||YES/NO||YES/NO|| 
    10 ||Do the proposed code changes correspond with the stated reason for the change?||YES/NO||YES/NO|| 
    11 ||Is the in-line documentation accurate and sufficient?||YES/NO||YES/NO|| 
    12 ||Do the code changes comply with NEMO coding standards?||YES/NO||YES/NO|| 
    13 ||Is the Ticket documented with sufficient detail for others to understand the impact of the change?||YES/NO||YES/NO|| 
    14 ||Does any corresponding external documentation require updating?||YES/NO||YES/NO|| 
    15 ||If yes, which docs and have the updates been drafted?||YES/NO||YES/NO|| 
    16 ||Are namelist changes required for this change?||YES/NO||YES/NO|| 
    17 ||If yes, have they been done?||YES/NO||YES/NO|| 
    18 ||Has a completed Ticket Summary template been appended to the ticket to aid code reviews||YES/NO||YES/NO|| 
    19 ||Does this summary correspond with your understanding of the full ticket?||YES/NO||YES/NO|| 
     9|| Do you understand the area of code being altered and the reasoning why it is being altered? || YES || YES/NO || 
     10|| Do the proposed code changes correspond with the stated reason for the change? || YES || YES/NO || 
     11|| Is the in-line documentation accurate and sufficient? || YES || YES/NO || 
     12|| Do the code changes comply with NEMO coding standards? || YES || YES/NO || 
     13|| Is the Ticket documented with sufficient detail for others to understand the impact of the change? || YES || YES/NO || 
     14|| Does any corresponding external documentation require updating? || YES || YES/NO || 
     15|| If yes, which docs and have the updates been drafted? || The OBS chapter in the NEMO book has been drafted. || YES/NO || 
     16|| Are namelist changes required for this change? || YES || YES/NO || 
     17|| If yes, have they been done? || YES || YES/NO || 
     18|| Has a completed Ticket Summary template been appended to the ticket to aid code reviews || YES || YES/NO || 
     19|| Does this summary correspond with your understanding of the full ticket? || YES || YES/NO || 
    2020 
    2121Ticket, Documentation and Code comments 
    2222 
    23 Add specific Ticket, Documentation and code comments here 
     23'''Reviewer 1''': No specific comments on the documentation. 
    2424 
    2525=== Testing === 
    26 ||Has the NVTK and other jobs been tested with this change?||YES/NO||YES/NO|| 
    27 ||Have the required bit comparability tests been run?||YES/NO||YES/NO|| 
    28 ||Can this change be shown to have a null impact? (if option not selected)||YES/NO||YES/NO|| 
    29 ||If no, is reason for the change valid/understood?||YES/NO/NA||YES/NO/NA|| 
    30 ||If no, ensure that the ticket details the impact this change will have on model configurations .||YES/NO/NA||YES/NO/NA|| 
    31 ||Is this change expected to preserve all diagnostics?||YES/NO||YES/NO|| 
    32 ||If no, is reason for the change valid/understood?||YES/NO/NA||YES/NO/NA|| 
    33 ||Are there significant changes in run time/memory?||YES/NO||YES/NO|| 
     26|| Has the NVTK and other jobs been tested with this change? || YES || YES/NO || 
     27|| Have the required bit comparability tests been run? || YES || YES/NO || 
     28|| Can this change be shown to have a null impact? (if option not selected) || YES || YES/NO || 
     29|| If no, is reason for the change valid/understood? || NA || YES/NO/NA || 
     30|| If no, ensure that the ticket details the impact this change will have on model configurations . || NA || YES/NO/NA || 
     31|| Is this change expected to preserve all diagnostics? || YES || YES/NO || 
     32|| If no, is reason for the change valid/understood? || NA || YES/NO/NA || 
     33|| Are there significant changes in run time/memory? || NO || YES/NO || 
    3434 
    3535Testing Comments 
    3636 
    37 Add specific testing comments here 
     37'''Reviewer 1''': The proposed SETTE tests give as outputs the same statistics in ocean.ouput than the previous OBS version on my own machines.  
    3838 
    3939Add specific testing comments here 
    4040 
    4141=== Code Review === 
    42 ||Do the code changes comply with NEMO coding standards?||YES/NO||YES/NO|| 
    43 ||Are code changes consistent with the design of NEMO?||YES/NO||YES/NO|| 
    44 ||Is the code free of unwanted TABs?||YES/NO||YES/NO|| 
    45 ||Has the code been wholly (100%) produced by NEMO developers working on NEMO?||YES/NO||YES/NO|| 
    46 ||If no, ensure collaboration agreement has been added to the ticket keywords|||| 
     42|| Do the code changes comply with NEMO coding standards? || YES || YES/NO || 
     43|| Are code changes consistent with the design of NEMO? || YES || YES/NO || 
     44|| Is the code free of unwanted TABs? || YES || YES/NO || 
     45|| Has the code been wholly (100%) produced by NEMO developers working on NEMO? || YES || YES/NO || 
     46|| If no, ensure collaboration agreement has been added to the ticket keywords || 
    4747 
    48 Add specific code comments or suggested alterations here. 
     48'''Reviewer 1''': I have one suggestion, more about the NEMO architecture. It seems to me more meaningful to extract the OBS module from the OPA_SRC directory.[[BR]]First, for me, it is not directly related to the NEMO dynamic core, then it can be used by other NEMO components (Sea-Ice component, for example) 
    4949 
    5050=== Review Summary === 
    51 Add summary here 
     51'''Reviewer 1''': These changes correspond clearly to a very welcomed simplification of the OBS module. They are well written, both for code and documentation, according to the NEMO coding conventions. 
    5252 
    5353=== Approval for the trunk === 
    54 Reviewer 1: YES/NO 
    55 Reviewer 2: YES/NO 
     54'''Reviewer 1''': YES Reviewer 2: YES/NO 
    5655 
    5756The code reviewer may approve the change for the NEMO trunk when: