Closed rodrigoguzman-lmd closed 6 years ago
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]
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
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?
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.
We need to understand this before we merge any changes to the master branch. @dustinswales do you know what's happened?
@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
@dustinswales @alejandrobodas Should I erase that extra cfmip2 folder, commit again and push to my repo?
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.
@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.
@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.
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?
Hi @alejandrobodas Sorry for that, I think the folder is clean now.
Thanks @rodrigoguzman-lmd , it is much cleaner now. I only have a few more minor requests:
@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.
@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 :smiley: Thanks for your help!
@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.
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
@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
@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)
@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.
@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.
@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?
@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.
@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!
documentation_OPAQ+GroundLidar+ATLID_COSPv2.pdf