Last edited Timestamp?

For completion by the Sci/Tech/Code? reviewer

Reviewer: Daniel Lea

Comments

Happy to review. I'll do a full review soon.

Some initial thoughts in response to https://forge.ipsl.jussieu.fr/nemo/browser/branches/2012/dev_r3342_MERCATOR7_SST revision 3436.

Looks like you are following the same style as the profile daily averaging which is a good idea. However, unless I am mistaken, both your code and the profile code are rather inefficient in that they allocate the vdmean array even if it is not needed. It would be nice if you could avoid allocating this array in obs_surf_def if the night-time averaging is not being used.

ld_sstnight should be set to false by default. Not all SST observation types may be suitable to be compared with the night-time average. I also think the expectation would be that an instantaneous observation would be compared to the model value at the same time (that is the case for all other observation types) so that should be the default and the ld_sstnight option available if you actively set it to true in the namelist. So you will need to add ln_sstnight to the namelist dia_obs_init.

I previously suggested that it be possible to select the SST types which use the night-time average. I'm happy to not worry about this providing the scheme is default to off as stated above. Others can develop it further as and when required.

Can you test that you get the same feedback files for SST and for other obs types (just to be sure), if you run with the NEMO trunk and with your branch but with with ld_sstnight = .false.?

Please indent the code inside the if statement in obs_sst_opt.

Daniel Lea 15/11/2012

==================== Response from Clement BRICAUD and Elisabeth Remy:

Concerning the ln_sstnight, we will put one logical in namelist for each SST to precise if it is a sst night or not: ln_reysst_night, ln_ghrsst_night and ln_sstfb_night Test with ld_sstnight = .false. has been already done and results are the same with NEMO_3.4 tag. For the indentation in obs_sst_opt, it is ok. Concerning the optionnal allocation for vdmean in obs_surf_alloc, it is a little bit heavy , because ln_*sst_night logical are declared in upper routine (diaobs), and it needs to put some optional logical in argument for obs_rea_sst* subroutines, and obs_surf_alloc, obs_surf_dealloc, obs_surf_compress,obs_surf_decompress subroutines.

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

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/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

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.
2. the above check-list has been completed.

or the code reviewer may choose to reject & assign the change back to the code author.