search

CFMIP / COSPv2.0

COSP - The CFMIP (Cloud Feedbacks Model Intercomparison Project) Observation Simulator Package
42 stars 38 forks source link

Fix indexing error in input argument to lidar_column. #38

Closed dustinswales closed 4 years ago

dustinswales commented 4 years ago

This PR contains a small bug-fix for an input to the LIDAR simulator. This was brought to our attention by Brian Eaton of NCAR when testing the latest COSP tag, v2.1.5, in CESM2 with the NAG compiler.

The field containing the "pressure at the interface levels" is being passed into lidar_Column() with the wrong extent. This error was introduced during PR #6, but seemed to slip through our testing.

RobertPincus commented 4 years ago

I looked this over. I'm not quite clear why we're using pressures at half-levels for an arguments that's supposedly the layer pressures, but otherwise the fix looks correct to me.

dustinswales commented 4 years ago

@RobertPincus This variable name is wrong. It contains the "pressure at model levels".

This is a naming convention that dates back to COSP1 (see line 90: https://github.com/CFMIP/COSPv1/blob/master/actsim/lmd_ipsl_stats.F90); however, that field is populated with the pressure at model levels (see line 92: https://github.com/CFMIP/COSPv1/blob/master/cosp_stats.F90)

In COSP2 the model at "pressure levels" is one dimension greater than the model at "pressure layers", hence the 2:Nlevels+1 extent in COSP2 to maintain consistency between COSP1 and COSP2 lidar/radar diagnostics.

rodrigoguzman-lmd commented 4 years ago

@dustinswales , @RobertPincus I wasn’t following COSPv2 changes closely lately, so I just saw this bug fix recently. I would like to shed some light on the variable concerned by this pull request, because I think it could be useful to solve issue #34 too, on which I will be working on soon.

Attached to this comment, you will find a figure ("COSPv2_variable_altitude_schemes.pdf") where simple schemes show at which altitude the different variable values correspond to. The schemes are based on variable names from the file https://github.com/CFMIP/COSPv2.0/blob/master/driver/src/cosp2_test.f90 and the few example values reported on the schemes correspond to the lower-altitude rounded values of the 10th profile from the inputs sample data given in COSPv2’s driver (file https://github.com/CFMIP/COSPv2.0/blob/master/driver/data/inputs/UKMO/cosp_input_um.nc).

Based on the schemes, which are based on the current "cosp2_test.f90" code (lines 471-486), the variable "cospgridIN%phalf" in file https://github.com/CFMIP/COSPv2.0/blob/master/src/cosp.F90 (corresponding to variable "cospstateIN%phalf" in "cosp2_test.f90") concerning the bug fix here does not seem to contain the "pressure at model levels" ("p" variable) as @dustinswales said in his previous comment, but indeed contains the "pressure at half model levels" ("ph" variable). @dustinswales , do you agree with me ? I didn’t try to understand what was done in COSPv1, but I would like to be sure that I’m not missing something in what we are doing in COSPv2.

And as shows figure "Pull_6_cosp.f90_highlighted.pdf" also attached to this comment, this error was not introduced during PR #6 as it is stated at the end of the first comment of this conversation (see the line highlighted by the red box, which was not modified by PR #6 ). COSPv2_variable_altitude_schemes.pdf Pull_6_cosp.f90_highlighted.pdf