Changes between Version 1 and Version 2 of ticket/0704/Review
- Timestamp:
- 2010-09-15T10:43:42+02:00 (14 years ago)
Legend:
- Unmodified
- Added
- Removed
- Modified
-
ticket/0704/Review
v1 v2 4 4 5 5 == For completion by the Sci/Tech/Code reviewer == 6 '''Reviewer:''' [ Enter your name here ] 6 '''Reviewer:''' [ Gurvan Madec ] 7 8 '''Ticket''' #704 9 10 '''NEMO Ticket page''' : DOES NOT EXIST ! 7 11 8 12 === 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||10 ||Do the proposed code changes correspond with the stated reason for the change?||YES /NO||11 ||Is the in-line documentation accurate and sufficient?||YES /NO||12 ||Do the code changes comply with NEMO coding standards?||YES /NO||13 ||Is the Ticket documented with sufficient detail for others to understand the impact of the change?|| YES/NO||14 ||Does any corresponding external documentation require updating?||YES /NO||15 ||If yes, which docs and have the updates been drafted?|| YES/NO||16 ||Are namelist changes required for this change?|| YES/NO||13 ||Do you understand the area of code being altered and the reasoning why it is being altered?||YES|| 14 ||Do the proposed code changes correspond with the stated reason for the change?||YES|| 15 ||Is the in-line documentation accurate and sufficient?||YES|| 16 ||Do the code changes comply with NEMO coding standards?||YES|| 17 ||Is the Ticket documented with sufficient detail for others to understand the impact of the change?||NO|| 18 ||Does any corresponding external documentation require updating?||YES|| 19 ||If yes, which docs and have the updates been drafted?||NO|| 20 ||Are namelist changes required for this change?||NO|| 17 21 ||If yes, have they been done?||YES/NO|| 18 ||Has a completed Ticket Summary template been appended to the ticket to aid code reviews||YES/NO||19 ||Does this summary correspond with your understanding of the full ticket?||YES/NO||22 ||Has a completed Ticket Summary template been appended to the Ticket #704 to aid code reviews||NO|| 23 ||Does this summary correspond with your understanding of the full Ticket #704?||YES/NO|| 20 24 21 25 Ticket, Documentation and Code comments … … 23 27 Add specific Ticket, Documentation and code comments here 24 28 29 The coding style have to be slightly improved. Suggestions of style modification have been given to the autor 30 31 Neither the ticket nor the associated (non exixting!) wiki page describe what has been changed and how to activate the changes 32 33 The paper documentation is missing, but should not be required as there is currently no ice documentation. 34 Nevertheless, a published paper (Bouillon et al ocean modelling 2009) describe in detail what has been done and which effect this has on the model solution. 35 36 37 38 25 39 === Testing === 26 ||Has the NVTK and other jobs been tested with this change?||YES/NO|| 27 ||Have the required bit comparability tests been run?||YES/NO|| 28 ||Can this change be shown to have a null impact? (if option not selected)||YES/NO|| 40 ||Has the NVTK and other jobs been tested with this change?||YES|| 41 ||Have the required bit comparability tests been run?||YES|| 42 ||Can this change be shown to have a null impact? (if option not selected)||NO|| 43 ||If no, is reason for the change valid/understood?||YES|| 44 ||If no, ensure that the ticket details the impact this change will have on model configurations .||YES/NO/NA|| 45 ||Is this change expected to preserve all diagnostics?||YES|| 29 46 ||If no, is reason for the change valid/understood?||YES/NO/NA|| 30 ||If no, ensure that the ticket details the impact this change will have on model configurations .||YES/NO/NA|| 31 ||Is this change expected to preserve all diagnostics?||YES/NO|| 32 ||If no, is reason for the change valid/understood?||YES/NO/NA|| 33 ||Are there significant changes in run time/memory?||YES/NO|| 47 ||Are there significant changes in run time/memory?||??|| 34 48 35 49 Testing Comments 36 50 37 Add specific testing comments here 51 No significant changes in the memory requirement. 52 No information given about the CPU time.... 38 53 39 Add specific testing comments here40 54 41 55 === Code Review === 42 ||Do the code changes comply with NEMO coding standards?||YES /NO||43 ||Are code changes consistent with the design of NEMO?|| YES/NO||44 ||Is the code free of unwanted TABs?||YES /NO||45 ||Has the code been wholly (100%) produced by NEMO developers working on NEMO?||YES /NO||56 ||Do the code changes comply with NEMO coding standards?||YES|| 57 ||Are code changes consistent with the design of NEMO?||almost YES|| 58 ||Is the code free of unwanted TABs?||YES|| 59 ||Has the code been wholly (100%) produced by NEMO developers working on NEMO?||YES|| 46 60 ||If no, ensure collaboration agreement has been added to the ticket keywords|||| 47 61 48 Add specific code comments or suggested alterations here. 62 The development can be better in term of modularity of the code. But this can be done latter on. 63 The improvement will consist in defining the rheology arrays in limrhg module in a new lim_rhg_init routine (not in ice.F90 module) 64 for both VP and EVP case. This reduce the number of time the key_VP appears in the system. 65 66 Nevertheless, I guess that within 1/2 years, the VP rheology will disappear... So there is no really need to perform this additional work 49 67 50 68 === Review Summary === 51 Add summary here 69 It is a well done work, the documentation is missing (as for all the LIM 2 and 3 components) but a paper (bouillon et al Ocean Modelling 2009) 70 describe precisely the VP and EVP rheology in LIM and the sensitivity of this change in LIM2 ! This should be sufficient. 52 71 53 72 === Approval for the trunk === 54 YES /NO73 YES if the suggested style modifications are done and a NEMOTicket page created on the wiki 55 74 56 75 The code reviewer may approve the change for the NEMO trunk when:
