Changes between Version 1 and Version 2 of ticket/1605/Review

Timestamp:

2015-10-07T12:14:47+02:00 (9 years ago)

Author:

pabouttier

Comment:

--

ticket/1605/Review

@@ -4 +4 @@
 
 == For completion by the Sci/Tech/Code reviewer ==
-'''Reviewer:''' [ Enter your name here ]
+'''Reviewer:''' 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||
-||Do the proposed code changes correspond with the stated reason for the change?||YES/NO||
-||Is the in-line documentation accurate and sufficient?||YES/NO||
-||Do the code changes comply with NEMO coding standards?||YES/NO||
-||Is the Ticket documented with sufficient detail for others to understand the impact of the change?||YES/NO||
-||Does any corresponding external documentation require updating?||YES/NO||
-||If yes, which docs and have the updates been drafted?||YES/NO||
-||Are namelist changes required for this change?||YES/NO||
-||If yes, have they been done?||YES/NO||
-||Has a completed Ticket Summary template been appended to the ticket to aid code reviews||YES/NO||
-||Does this summary correspond with your understanding of the full ticket?||YES/NO||
+|| 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? || Partially, due to the storng.F90 file (see https://forge.ipsl.jussieu.fr/nemo/ticket/1605 ) ||
+|| 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? || A chapter in the NEMO book is missing about that module. ||
+|| Are namelist changes required for this change? || YES ||
+|| If yes, have they been done? || YES (yes, in the CONFIG/SHARED/namelist_ref file) ||
+|| Has a completed Ticket Summary template been appended to the ticket to aid code reviews || NO (see https://forge.ipsl.jussieu.fr/nemo/ticket/1605 ) ||
+|| Does this summary correspond with your understanding of the full ticket? || NA ||
 
 Ticket, Documentation and Code comments
 
-Add specific Ticket, Documentation and code comments here
+The main point here is that the documentation in the NEMO book is missing.
 
 === Testing ===
-||Has the NVTK and other jobs been tested with this change?||YES/NO||
-||Have the required bit comparability tests been run?||YES/NO||
-||Can this change be shown to have a null impact? (if option not selected)||YES/NO||
-||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/NO||
-||If no, is reason for the change valid/understood?||YES/NO/NA||
-||Are there significant changes in run time/memory?||YES/NO||
+|| Has the NVTK and other jobs been tested with this change? || YES ||
+|| 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? || NA ||
+|| If no, ensure that the ticket details the impact this change will have on model configurations . || NA ||
+|| Is this change expected to preserve all diagnostics? || YES (if option not selected) ||
+|| If no, is reason for the change valid/understood? || NA ||
+|| Are there significant changes in run time/memory? || NO (if option not selected) ||
 
 Testing Comments
 
-Add specific testing comments here
-
-Add specific testing comments here
+SETTE tests are all OK when option is disabled. Scientific tests have not been done with this module enabled.
 
 === 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||||
+|| Do the code changes comply with NEMO coding standards? || Partially (as previously said) ||
+|| 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.
+Allocation of arrays in the module are not checked during the initialisation phase. This have to be fixed.
+
+Another remark is more about  the software architecture. It seems to be more meaningful to extract STO/ directory from OPA_SRC directory. Indeed, the module is not directly related to the NEMO dynamic core and it is generic enough to be applied to other NEMO components (e.g. LIM2, PISCES)
 
 === Review Summary ===
-Add summary here
+Currently, the stochastic parametrization of EOS is implemented. It does not introduce regressive behaviors of the code. 
+
+However, an external documentation is still missing. Also, Scientific tests have not been done. 
+
+Concerning the code itself, arrays allocation have to be checked. The STO directory could be extracted from OPA_SRC.
 
 === Approval for the trunk ===