Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug fixes from Brian Eaton at NCAR #5

Merged
merged 6 commits into from
Mar 12, 2018
Merged

Bug fixes from Brian Eaton at NCAR #5

merged 6 commits into from
Mar 12, 2018

Conversation

RobertPincus
Copy link
Contributor

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.

  • Robert

@alejandrobodas
Copy link
Collaborator

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
Copy link
Contributor Author

RobertPincus commented Feb 1, 2018 via email

@alejandrobodas
Copy link
Collaborator

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
Copy link
Contributor

dustinswales commented Feb 2, 2018

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 (fe4be9f)

Dustin
cosp2.spVdp.deltas.txt

@klein21
Copy link

klein21 commented Feb 2, 2018 via email

@RobertPincus
Copy link
Contributor Author

RobertPincus commented Feb 2, 2018 via email

@dustinswales
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor

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

@dustinswales
Copy link
Contributor

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

Copy link
Collaborator

@alejandrobodas alejandrobodas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Robert, Dustin, thanks for this. I have no more comments on this, so please go ahead with the merge.

@dustinswales dustinswales merged commit 56aee9a into master Mar 12, 2018
@dustinswales dustinswales deleted the ncar-fixes branch March 12, 2018 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants