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

Timestamp:

2015-10-05T09:48:16+02:00 (9 years ago)

Author:

bouttier

Comment:

--

ticket/1468/Review

@@ -4 +4 @@
 
 == For completion by the Sci/Tech/Code reviewer ==
-'''Reviewer:''' [ Enter your name here ]
+'''Reviewer 1:''' Pierre-Antoine Bouttier
 
 === Ticket Details, Documentation and Code changes ===
-||Do you understand the area of code being altered and the reasoning why it is being altered?||YES/NO||YES/NO||
-||Do the proposed code changes correspond with the stated reason for the change?||YES/NO||YES/NO||
-||Is the in-line documentation accurate and sufficient?||YES/NO||YES/NO||
-||Do the code changes comply with NEMO coding standards?||YES/NO||YES/NO||
-||Is the Ticket documented with sufficient detail for others to understand the impact of the change?||YES/NO||YES/NO||
-||Does any corresponding external documentation require updating?||YES/NO||YES/NO||
-||If yes, which docs and have the updates been drafted?||YES/NO||YES/NO||
-||Are namelist changes required for this change?||YES/NO||YES/NO||
-||If yes, have they been done?||YES/NO||YES/NO||
-||Has a completed Ticket Summary template been appended to the ticket to aid code reviews||YES/NO||YES/NO||
-||Does this summary correspond with your understanding of the full ticket?||YES/NO||YES/NO||
+|| Do you understand the area of code being altered and the reasoning why it is being altered? || YES || YES/NO ||
+|| Do the proposed code changes correspond with the stated reason for the change? || YES || YES/NO ||
+|| Is the in-line documentation accurate and sufficient? || YES || YES/NO ||
+|| Do the code changes comply with NEMO coding standards? || YES || YES/NO ||
+|| Is the Ticket documented with sufficient detail for others to understand the impact of the change? || YES || YES/NO ||
+|| Does any corresponding external documentation require updating? || YES || YES/NO ||
+|| If yes, which docs and have the updates been drafted? || The OBS chapter in the NEMO book has been drafted. || YES/NO ||
+|| Are namelist changes required for this change? || YES || YES/NO ||
+|| If yes, have they been done? || YES || YES/NO ||
+|| Has a completed Ticket Summary template been appended to the ticket to aid code reviews || YES || YES/NO ||
+|| Does this summary correspond with your understanding of the full ticket? || YES || YES/NO ||
 
 Ticket, Documentation and Code comments
 
-Add specific Ticket, Documentation and code comments here
+'''Reviewer 1''': No specific comments on the documentation.
 
 === Testing ===
-||Has the NVTK and other jobs been tested with this change?||YES/NO||YES/NO||
-||Have the required bit comparability tests been run?||YES/NO||YES/NO||
-||Can this change be shown to have a null impact? (if option not selected)||YES/NO||YES/NO||
-||If no, is reason for the change valid/understood?||YES/NO/NA||YES/NO/NA||
-||If no, ensure that the ticket details the impact this change will have on model configurations .||YES/NO/NA||YES/NO/NA||
-||Is this change expected to preserve all diagnostics?||YES/NO||YES/NO||
-||If no, is reason for the change valid/understood?||YES/NO/NA||YES/NO/NA||
-||Are there significant changes in run time/memory?||YES/NO||YES/NO||
+|| Has the NVTK and other jobs been tested with this change? || YES || YES/NO ||
+|| Have the required bit comparability tests been run? || YES || YES/NO ||
+|| Can this change be shown to have a null impact? (if option not selected) || YES || YES/NO ||
+|| If no, is reason for the change valid/understood? || NA || YES/NO/NA ||
+|| If no, ensure that the ticket details the impact this change will have on model configurations . || NA || YES/NO/NA ||
+|| Is this change expected to preserve all diagnostics? || YES || YES/NO ||
+|| If no, is reason for the change valid/understood? || NA || YES/NO/NA ||
+|| Are there significant changes in run time/memory? || NO || YES/NO ||
 
 Testing Comments
 
-Add specific testing comments here
+'''Reviewer 1''': The proposed SETTE tests give as outputs the same statistics in ocean.ouput than the previous OBS version on my own machines. 
 
 Add specific testing comments here
 
 === Code Review ===
-||Do the code changes comply with NEMO coding standards?||YES/NO||YES/NO||
-||Are code changes consistent with the design of NEMO?||YES/NO||YES/NO||
-||Is the code free of unwanted TABs?||YES/NO||YES/NO||
-||Has the code been wholly (100%) produced by NEMO developers working on NEMO?||YES/NO||YES/NO||
-||If no, ensure collaboration agreement has been added to the ticket keywords||||
+|| Do the code changes comply with NEMO coding standards? || YES || YES/NO ||
+|| Are code changes consistent with the design of NEMO? || YES || YES/NO ||
+|| Is the code free of unwanted TABs? || YES || YES/NO ||
+|| Has the code been wholly (100%) produced by NEMO developers working on NEMO? || YES || YES/NO ||
+|| If no, ensure collaboration agreement has been added to the ticket keywords ||
 
-Add specific code comments or suggested alterations here.
+'''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)
 
 === Review Summary ===
-Add summary here
+'''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.
 
 === Approval for the trunk ===
-Reviewer 1: YES/NO
-Reviewer 2: YES/NO
+'''Reviewer 1''': YES Reviewer 2: YES/NO
 
 The code reviewer may approve the change for the NEMO trunk when: