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

Adding CALIPSO-OPAQ new diagnostics and GROUND LIDAR + ATLID simulators #11

Merged
merged 18 commits into from
Jun 1, 2018

Conversation

rodrigoguzman-lmd
Copy link
Contributor

@rodrigoguzman-lmd
Copy link
Contributor Author

When running regression test from the python script given in COSPv2.0/driver :

[rguzman@loholt2 driver]$ pwd
/bdd/CFMIP/workspace/rguzman/COSPv2.0/driver

[rguzman@loholt2 driver]$ python test_cosp2Imp.py data/outputs/UKMO/cosp2_output_um.ref.nc data/outputs/UKMO/cosp2_output_um.nc
############################################################################################
Treating relative differences less than 0.0010000000% as insignificant
bnds: 100.00 % of values differ, relative range: 2.70e+01 to 5.40e+01
atb532_perp: 0.15 % of values differ, relative range: -1.31e-05 to 1.84e-05
atb532: 0.15 % of values differ, relative range: -1.31e-05 to 1.82e-05
lidarBetaMol532: 0.15 % of values differ, relative range: -1.31e-05 to 1.82e-05
dbze94: 0.17 % of values differ, relative range: -1.77e-04 to 1.37e-04
cltmodis: 0.65 % of values differ, relative range: 5.00e-01 to 5.00e-01
tautmodis: 0.65 % of values differ, relative range: -3.17e-01 to -3.17e-01
tautlogmodis: 0.65 % of values differ, relative range: -5.31e-01 to -5.31e-01
pctmodis: 0.65 % of values differ, relative range: -1.93e-01 to -1.93e-01
All other fields match.

“Bnds” values in outputs:

cosp2_output_um.nc [55, 56]
cosp2_output_um.ref.nc [1, 2]

@rodrigoguzman-lmd
Copy link
Contributor Author

@rodrigoguzman-lmd
Copy link
Contributor Author

2D_variables_figures.pdf

@rodrigoguzman-lmd
Copy link
Contributor Author

The Python scripts attached to this comment were used to plot all of the figures found in the documents "CF3D_betamol_SRhisto_figures.pdf" and "2D_variables_figures.pdf" attached in the two previous comments. No 2D map outputs were available so all the plots of "2D_variables_figures.pdf" are diagnoctic values as a function of the location number (153) and all the plots of "CF3D_betamol_SRhisto_figures.pdf" are mean profiles or sum of occurrences of the 153 locations.
Python_figure_scripts.tar.gz

@alejandrobodas
Copy link
Collaborator

Hi @rodrigoguzman-lmd . Thanks for this. I've skimmed through the changes and I see some files that appear to be changed, although they don't relate to this particular change (e.g. cfmip2 namelists). Is that a mistake?

@rodrigoguzman-lmd
Copy link
Contributor Author

Hi @alejandrobodas . I did not modified those files, so I don't understand why these files appear as been changed. Maybe I did something wrong at some point. All the files that I modified are listed in the document of my very first comment of this pull request.

@alejandrobodas
Copy link
Collaborator

We need to understand this before we merge any changes to the master branch. @dustinswales do you know what's happened?

@dustinswales
Copy link
Contributor

@alejandrobodas @rodrigoguzman-lmd
I took a look at the commit history and appears that the directory cfmip2/ was copied into itself (e.g. cfmip2/cfmip2/) on your March 19 commit:
https://github.com/CFMIP/COSPv2.0/tree/2205318a0c159d05ff9c3d6902ed6fab2417b3ed/cfmip2
https://github.com/CFMIP/COSPv2.0/tree/de92f96fe816da68f935fe4d61115c4a535ead46/cfmip2

@rodrigoguzman-lmd
Copy link
Contributor Author

@dustinswales @alejandrobodas Should I erase that extra cfmip2 folder, commit again and push to my repo?

@rodrigoguzman-lmd
Copy link
Contributor Author

Ok, I moved the files from the "cfmip2/cfmip2" extra folder to the "cfmip2" folder and also moved the files from the "cosp-interface/cosp-interface" extra folder to the "cosp-interface" folder. I'm doing a new commit and pushing to my repo.

@dustinswales
Copy link
Contributor

@rodrigoguzman-lmd
I have gone through the code and I have one comment.

I noticed that for each new Lidar platform there are new source files (COSPv2.0/src/simulator/actsim/), and new example routines to compute the optical inputs (subsample_and_optics_example/optics/cosp_optics.F90). It appears that it would be easy to generalize these into a common lidar simulator that can be configured for multiple platforms.
For example, in cosp_optics.F90 the subroutines lidar_optics, atlid_optics, and grounlidar_optics could be combined into one subroutine by making a few small changes. I haven't looked deeply at the simulator routines, but I would imagine the same could be said about those as well.

@rodrigoguzman-lmd
Copy link
Contributor Author

@dustinswales Thank you for your comment. I have managed to merge the "atlid" and "groundlidar" simulator into one generic "lidar" simulator as you suggested. The "calipso" simulator having too many specificities (cloud phase, OPAQ variables) is still a simulator on its own. New lidar platforms can easily be defined from this new generic lidar simulator. All of the results shown in the documents/comments above are unchanged in this new version of the code.

@alejandrobodas
Copy link
Collaborator

Hi @rodrigoguzman-lmd There are still many files that appear as modified that have no relationship with this change (e.g. cosp_errorHandling.F90, cosp_defs.h, and others). Also, .mod and object files from your local build have been added to the repository. Please can you tidy things up a bit more?

@rodrigoguzman-lmd
Copy link
Contributor Author

Hi @alejandrobodas Sorry for that, I think the folder is clean now.

@alejandrobodas
Copy link
Collaborator

Thanks @rodrigoguzman-lmd , it is much cleaner now. I only have a few more minor requests:

  1. I don't think that many of the end-of-line comments are necessary: !OPAQ, !TIBO, !TIBO2, etc. For instance, in cosp2_output_nl.txt it is sufficient to leave the comments that separate blocks of diagnostics.
  2. Also, the comments encapsulating the changes (e.g. !BEGINNING OF CHANGES... !END OF CHANGES...) are also redundant. We will be able to track the changes using the history of changes in the repository, and these comments will soon become obsolete as we add new modifications.
  3. Please can you make sure that you define the real constants using _wp (e.g. L1213, and around L1257 in lidar_simulator.F90). We've seen some compilers complaining about this in the past.

@dustinswales
Copy link
Contributor

@rodrigoguzman-lmd
I updated the COSP2 master branch today with some improvements to COSP's internal error checking routine, cosp_errorCheck. There is also a small change to the subroutine cosp_init.

The main change is that cosp_errorCheck has an added layer of protection to guard against spurious inputs. So for example, if the inputs provided to COSP are not consistent with the diagnostics requested, the simulator is turned off, requested diagnostics are set to fill values, and an error message is provided.

You will need to update your repo with the main COSP2 repo. Luckily for you... I started implementing your diagnostics in CESM2 and did this the other day (see what I did in cosp_errorCheck of https://github.com/CFMIP/COSPv2.0/blob/lmd-newLidarDiag/src/cosp.F90). I'm sure there's a slick way in git to update a local file in your repository with a file in a remote repository.

@rodrigoguzman-lmd
Copy link
Contributor Author

@alejandrobodas Thanks for your last minor comments, I have taken into account all 3 of them.
@dustinswales Unfortunately, I did not manage to find a slick way to update my local file with yours, but I did manage to implemented your changes of cosp_errorCheck into my branch. Everything works correctly and all of the results shown previously are unchanged. Now, it seems that there are some conflicts that need to be resolved because of this last update. Knowing that I get the following information in my machine when I do:
[rguzman@loholt2 COSPv2.0]$ git remote -v
origin https://github.com/rodrigoguzman-lmd/COSPv2.0.git (fetch)
origin https://github.com/rodrigoguzman-lmd/COSPv2.0.git (push)
upstream https://github.com/CFMIP/COSPv2.0.git (fetch)
upstream https://github.com/CFMIP/COSPv2.0.git (push)

What shall I do (type) in order to resolve these conflicts? I don't want to mess everything up being so close to the goal 😃 Thanks for your help!

@dustinswales
Copy link
Contributor

@rodrigoguzman-lmd
I just merged the master onto your branch, all conflicts resolved. Now everything looks in order.

To recap,
*) You have made changes to the following source files:
src/cosp.F90
src/cosp_config.F90
src/simulator/actsim/lidar_simulator.F90
*) Added the following new source files:
src/simulator/actsim/lidar_simulator_nophase.F90
src/simulator/cosp_lidar_interface.F90
*) Modified source files for the offline driver accordingly (All offline tests pass).

I think we are really close, I may take a look at further generalizing the lidar simulators. I think we can combine them further with the use of optional arguments.

@rodrigoguzman-lmd
Copy link
Contributor Author

Updated figure files after bug correction on COSP_OPAQ subroutine (file "lidar_simulator.F90").
CF3D_betamol_SRhisto_figures_new.pdf
2D_variables_figures_new.pdf

@dustinswales
Copy link
Contributor

@rodrigoguzman-lmd
I ran into an issue running CESM2 with your new diagnostics. Shortly into the run the job is terminated with the following error:

forrtl: severe (408): fort: (3): Subscript #2 of the array TMP has value 0 which is less than the lower bound of 1
Image PC Routine Line Source
cam 0000000005E4F296 Unknown Unknown Unknown
cam 0000000005C7E8DB mod_lidar_simulat 1228 lidar_simulator.F90
cam 0000000005C225FA mod_lidar_simulat 360 lidar_simulator.F90

I tracked this down to some logic in one of your new subroutines, cosp_opaq in https://github.com/rodrigoguzman-lmd/COSPv2.0/blob/964cb7267f7e6cd1bdcda23d99106dc01c02b18c/src/simulator/actsim/lidar_simulator.F90.

First you compute some cloud masks.
@lines1149-1153: compute 3D (columns, sub-columns, levels) cloud mask, cldy.
@lines1155-1160: compute 3D (columns, sub-columns, levels) opaque cloud mask, cldyopaq.
@line1184: compute 2D (columns, sub-columns) opaque cloud mask, cldlay(:,:,1)
Then a little bit further down in the code, if there is an opaque cloud (e.g. if (cldlay(:,:,1) .eq. true)) you do a loop in the vertical to determine the vertical indices for the top/bottom of the opaque cloud. Later on the code crashes because these indices haven't been computed correctly. There are two errors in this loop, at least as far as I can tell, please correct me if I'm not interpreting your code correctly.

First, the all-cloud mask, "cldy" is being used to determine the vertical indices for the opaque cloud., whereas the opaque cloud mask should be used, "cldyopaq".
Second, when computing these indices, the loop is over 1:nlevels-1, then in the loop you invert the indices (e.g. nlevels-K), so you are going from SFC+1->TOA. However, in this case there is an opaque cloud in the lowermost level, which you never check.

I put some print statements in CESM2 to confirm this (level 1 = TOA):
zopac=0
z_top = 0
cldlay = 1
Level cldy cldyopaq Lidar Scattering Ratio
1 0.00000 0.00000 1.00000
2 0.00000 0.00000 1.00000
3 0.00000 0.00000 1.00000
4 0.00000 0.00000 1.00000
5 0.00000 0.00000 1.00000
6 0.00000 0.00000 1.00000
7 0.00000 0.00000 1.00000
8 0.00000 0.00000 1.00000
9 0.00000 0.00000 1.00000
10 0.00000 0.00000 1.00000
11 0.00000 0.00000 1.00000
12 0.00000 0.00000 1.00000
13 0.00000 0.00000 1.00000
14 0.00000 0.00000 1.00000
15 0.00000 0.00000 1.00000
16 0.00000 0.00000 1.00000
17 0.00000 0.00000 1.00000
18 0.00000 0.00000 1.00000
19 0.00000 0.00000 1.00000
20 0.00000 0.00000 1.00000
21 0.00000 0.00000 1.08612
22 0.00000 0.00000 1.10412
23 0.00000 0.00000 2.27624
24 0.00000 0.00000 2.57670
25 0.00000 0.00000 3.31739
26 0.00000 0.00000 3.63397
27 0.00000 0.00000 4.01202
28 0.00000 0.00000 4.36650
29 0.00000 0.00000 4.40758
30 0.00000 0.00000 4.54373
31 0.00000 0.00000 4.54373
32 0.00000 0.00000 4.24441
33 0.00000 0.00000 4.19809
34 0.00000 0.00000 4.08831
35 0.00000 0.00000 4.07476
36 0.00000 0.00000 4.45716
37 0.00000 0.00000 3.73582
38 0.00000 0.00000 4.55651
39 0.00000 0.00000 3.98720
40 0.00000 1.00000 0.0190253

@dustinswales
Copy link
Contributor

@rodrigoguzman-lmd
I made some small changes and tidied up. All regression tests passed.

My intention with these changes is just to make the code a bit more simple, easier to read, and (hopefully) easy to extend for future users who may want to add more lidar simulators.

I removed the "_nophase" functions/modules and added optional input/outputs to the original lidar routines. The " nophase" code is really just a subset of the original lidar simulator, so there's no need in having these lines of code repeated. I adopted a more general naming convention for the lidar variables, we now have three lidar simulators... So for example, the backscatter coefficients used by the calipso lidar simulator previously weren't identified by instrument in COSP (e.g cospIN%betatot), whereas for the new lidar simulators they are (e.g. cospIN%betatot_atlid and cospIN%betatot_grLidar532). Below is a piece of the cosp input derived type to illustrate:
real(wp),allocatable,dimension(:,:,:) :: &
frac_out, & ! Cloud fraction
tau_067, & ! Optical depth @ 0.67micron
emiss_11, & ! Emissivity @ 11 micron
fracLiq, & ! Fraction of optical-depth due to liquid (MODIS)
asym, & ! Assymetry parameter @ 3.7micron (MODIS)
ss_alb, & ! Single-scattering albedo @ 3.7micron (MODIS)
betatot_calipso, & ! Lidar backscatter coefficient (calipso @ 532nm)
betatot_grLidar532, & ! Lidar backscatter coefficient (ground-lidar @ 532nm)
betatot_atlid, & ! Lidar backscatter coefficient (atlid @ 355nm)
betatot_ice_calipso, & ! Lidar backscatter coefficient ICE (calipso @ 532nm)
betatot_liq_calipso, & ! Lidar backscatter coefficient LIQUID (calipso @ 532nm)
tautot_calipso, & ! Lidar Optical thickness (calipso @ 532nm)
tautot_grLidar532, & ! Lidar Optical thickness (ground-lidar @ 532nm)
tautot_atlid, & ! Lidar Optical thickness (atlid @ 355nm)
tautot_ice_calipso, & ! Lidar Ice Optical thickness (calipso @ 532nm)
tautot_liq_calipso, & ! Lidar Liquid Optical thickness (calipso @ 532nm)
z_vol_cloudsat, & ! Effective reflectivity factor (mm^6/m^3)
kr_vol_cloudsat, & ! Attenuation coefficient hydro (dB/km)
g_vol_cloudsat ! Attenuation coefficient gases (dB/km)

@rodrigoguzman-lmd
Copy link
Contributor Author

@dustinswales
All these small changes look great, and they will indeed allow to introduce more easily new lidar simulators, thanks.
Please let me know if the patch I sent you recently by email to debug the issue you ran into when you were running these new diagnostics in CESM2 is working fine.

@dustinswales
Copy link
Contributor

@rodrigoguzman-lmd
Thanks for the patch. I will test this in CESM2.
For future reference, instead of a patch, it's easier if you just make the change and update your branch. In CESM2, I'm accessing your COSP2 developmental branch as an external, so if you update your branch, I can immediately see these changes in CESM2.

@dustinswales
Copy link
Contributor

@rodrigoguzman-lmd
I tested your patch and this does the trick!
However, I ran into another error in the PIO section. I was able to track this back to one of the output fields, cldtypemeanzse, in the routine cosp_opaq. The field was not initialized, so it contained some garbage and was failing during output. I initialized this field and everything appears to be working.
At this point I have no objections to merging your changes onto the trunk. @alejandrobodas, do you agree?

@alejandrobodas
Copy link
Collaborator

@rodrigoguzman-lmd @dustinswales
I have no objections. This change has gone through a much more thorough testing than our offline tests, which has caught several problems. Thank you both for your hard work on this.

@rodrigoguzman-lmd
Copy link
Contributor Author

@dustinswales @alejandrobodas
That's great. Thank you very much to both of you for your reviews of the code and for your comments that have allow to significantly improve the final implementation of these new lidar diagnostics and simulators. And thanks a lot @dustinswales for helping me find those last bugs and for correcting them!

@dustinswales dustinswales merged commit 91f2f92 into CFMIP:master Jun 1, 2018
dustinswales added a commit that referenced this pull request Jun 3, 2020
Adding CALIPSO-OPAQ new diagnostics and GROUND LIDAR + ATLID simulators
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.

None yet

3 participants