CFMIP / COSPv2.0

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

Bug fixes from Brian Eaton at NCAR #5

Closed RobertPincus closed 6 years ago

RobertPincus commented 6 years ago

I propose three changes suggested by Brian Eaton at NCAR based on experience with COSP 2 in CESM 2. 1) Fixing an indexing bug in cosp.F90 that's caught by strict compilers (Marston Johnson of SMHI also found this). 2) Removing a stray print statement in COSP, in keeping with the no-printing approach for COSP. 3) Changing the working precision from single-precision to double-precision by default.

Unfortunately my text editor also removed many trailing spaces so the default diffs look far more substantial.

alejandrobodas commented 6 years ago

Hello Robert,

I've skimmed through the changes, and I couldn't find the indexing bug . Please can you point me to the lines of the relevant changes in cosp.F90? Also, is it possible to add the outputs of the tests to this discussion? It would be good to have them saved here.

Thanks, Alejandro

RobertPincus commented 6 years ago

Alejandro -

It’s on line 889 of cosp.f90. Sorry, I know that my editor’s reformatting buries the important change in a mountain of changed white space.

Dustin can comment but I don’t expect the tests to have changed. I will take the point that I should mention this in all pull requests.

On Feb 1, 2018, at 10:24 AM, alejandrobodas notifications@github.com<mailto:notifications@github.com> wrote:

Hello Robert,

I've skimmed through the changes, and I couldn't find the indexing bug . Please can you point me to the lines of the relevant changes in cosp.F90? Also, is it possible to add the outputs of the tests to this discussion? It would be good to have them saved here.

Thanks, Alejandro

alejandrobodas commented 6 years ago

I see it now, thanks. The changes are trivial, but I expect the change in the default working precision to change the outputs. If that's the case, then the reference outputs should be updated with this change. Alejandro

Alejandro

dustinswales commented 6 years ago

Hi Robert and Alejandro,

The change in working-precision does make an impact on the output. Results from the testing script are attached. I've gone ahead and updated the reference files (fe4be9ff9da125fa574df2b152c0ec14052cfc4a)

Dustin cosp2.spVdp.deltas.txt

klein21 commented 6 years ago

Dustin,

You should investigate whether the changes are reasonable. While many relative changes are tiny, there are some which are of order 1 which is worrisome.

Steve

From: dustinswales notifications@github.com Reply-To: "CFMIP/COSPv2.0" reply@reply.github.com Date: Friday, February 2, 2018 at 10:03 AM To: "CFMIP/COSPv2.0" COSPv2.0@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: Re: [CFMIP/COSPv2.0] Bug fixes from Brian Eaton at NCAR (#5)

Hi Robert and Alejandro,

The change in working-precision does make an impact on the output. Results from the testing script are attached. I've gone ahead and updated the reference files

Dustin cosp2.spVdp.deltas.txthttps://github.com/CFMIP/COSPv2.0/files/1690205/cosp2.spVdp.deltas.txt

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/CFMIP/COSPv2.0/pull/5#issuecomment-362649504, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AfO_cNuumdVOxYJFf5x-BAFemDCNNhWBks5tQ0UYgaJpZM4R1rFH.

RobertPincus commented 6 years ago

The change in working-precision does make an impact on the output. Results from the testing script are attached. I've gone ahead and updated the reference files

Alejandro -

It would terrific if you could ensure offline that the changes are suitable and then let us know if you agree with our merging the changes onto master.

I would not expect to tag this release.

dustinswales commented 6 years ago

All,

Attached you will find plots detailing the differences I reported earlier. I only included plots for fields with differences greater than 1e-3. "green" points are where relative differences are insignificant, whereas "red" points indicate differences. Dustin

cosp2_ncar-fixes.pdf

alejandrobodas commented 6 years ago

Dustin, thanks for these plots, they are very useful. They suggest that the test cases that are flagged as red contain one extra cloudy subcolumn. You can see that because the Y values of the red points in cltmodis coincide with the next X values of the green points (whose distribution is quantized because the number of columns is discrete). In principle, this could point to a different behaviour of the random generator, but the fact that the same errors are not seen in the ISCCP diagnostics suggests that the differences are caused by something internal to the MODIS simulator. I believe the standard tests are run with -O3 optimisation level. Would it be possible to compare the effect of the change in working precision without optimisations?

Cheers, Alejandro

dustinswales commented 6 years ago

Hi Alejandro, I just ran the test case again without optimization and the results have not changed. Dustin

dustinswales commented 6 years ago

Alejandro,

As per your suggestion, I reverted the change in default precision from double back to single. To summarize, below are the changes in the branch ncar-fixes which we propose to merge onto the master branch, none of which change any of the COSP diagnostics: ) Remove unnecessary print statement (@line 99 in src/simulator/cosp_cloudsat_interface.F90) ) Fix indexing bug in call to lidar_subcolumn (@line 889 of src/cosp.F90) *) Move computation of the gaseous attenuation from quickbeam_optics to the model interface level.

Dustin