CFMIP / COSPv2.0

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

Bugfix in treatment of non-sunlit columns in warm rain diagnostics #70

Closed takmichibata closed 2 years ago

takmichibata commented 2 years ago

The warm rain diagnostics did intend to use only daytime cloudy scenes, but actually the current code did not explicitly exclude dark-scene columns from the analysis. To fix this bug, this pull request modifies the treatment of dark-scene columns as suggested by @brhillman and @dustinswales at the previous PR from 2019 (#36).

alejandrobodas commented 2 years ago

@takmichibata thanks for implementing this fix. This doesn't pass the tests because it changes the results, so we need to update the KGOs. Before we do that, we need to make sure that the changes are working as expected. The variables that fail the test are: npdfcld, npdfdrz, npdfrain, ncfodd1, ncfodd2, ncfodd3. There is more detailed information in the output of the tests for this PR. Are these all the variables that you'd expect this change to affect? Are the changes what you expected? You can find the new results and summary plots in the artifact produced by the CI tests (outputs.UKMO.tgz). If the CI test doesn't produce plots for the relevant variables (or the plots don't show what you need), please can you produce the relevant plots comparing the old and new results? Implementing modifications that change the results involves some additional steps, so we'll probably need a few iterations to get it right, so bear with us! @brhillman @dustinswales you were involved in the original discussion about masking, please would you mind reviewing the changes?

takmichibata commented 2 years ago

@alejandrobodas @dustinswales Thank you for your code review and comments. I attach the plots of npdf*, which are affected by this revision PR #70. As we expected, the revised version (b) has UNDEF value, where the MODIS LWP (c) is set to UNDEF. The previous version (a) was set to 0 even though the MODIS LWP is UNDEF. I feel that the revised code is now working appropriately. Could I replace the KGO file?

cosp2_pr70_rev1
brhillman commented 2 years ago

Sorry, just seeing this. The changes look good to me.

alejandrobodas commented 2 years ago

@takmichibata I've uploaded the new KGO to my google drive. These are modifications that you'll need to implement in your branch.

In .github/workflows/continuous_integration.yml:

A) replace the long 'id' strings at the end of the variables GDFILE with the following ids: Line 194: 1s5Ha6Hqnv_hWbRUs8KQpJ4Lxy8uvJDar Line 201: 11dKcIL3EQr7s6jbo4f9GsoW0SufesGbq Line 208: 1kY1lRgzd0UhDiQef2u-VgTQql_iut3U2

B) In line 46, change KGO_VERSION to v002

Add the following files to driver/data/outputs/UKMO/ , but renaming them without the .txt extension: cosp2_output.um_global.gfortran.kgo.v002.nc.md5.txt cosp2_output.um_global_model_levels.gfortran.kgo.v002.nc.md5.txt cosp2_output_um.gfortran.kgo.v002.nc.md5.txt

I think that's all. Commit your changes and let's hope it works!

takmichibata commented 2 years ago

@dustinswales @brhillman Thank you for kindly checking the revision. @alejandrobodas I appreciate your support, and I understood how to commit the change. Linux machines in my labo are temporarily stopped due to maintenance this weekend, so I'll do the commit at beginning of next week.

takmichibata commented 2 years ago

@alejandrobodas I updated the KGO version, and then commit the change. But the CI test finished with errors. Should I also upload the output files (driver/data/outputs/UKMO/)?

alejandrobodas commented 2 years ago

Hi @takmichibata I think the issue is that you are working from a version that is behind the master, and the line numbers I sent you didn't match what you have. I don't know know what's the easiest way to upgrade to the latest version of the master. If you don't know either, perhaps @dustinswales might know? Then, once you've done that, these are the lines to change: L195: 1s5Ha6Hqnv_hWbRUs8KQpJ4Lxy8uvJDar L202: 11dKcIL3EQr7s6jbo4f9GsoW0SufesGbq L209: 1kY1lRgzd0UhDiQef2u-VgTQql_iut3U2 Thanks!

dustinswales commented 2 years ago

Sorry, I checked out Taks code and meant to push to my forked repo, but accidentally pushed to the CFMIP repo. reopening