-
Notifications
You must be signed in to change notification settings - Fork 150
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
cam6_4_005: Limit vertical domain used by COSP #1010
Conversation
dtau_s(1:ncol,1:pver),dem_c(1:ncol,1:pver), & | ||
dem_s(1:ncol,1:pver),dtau_s_snow(1:ncol,1:pver), & | ||
dem_s_snow(1:ncol,1:pver),state%ps(1:ncol),cospstateIN,cospIN) | ||
call subsample_and_optics( & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to have a comment that subsample_and_optics works on only the active columns. Arrays that are dimensioned by pcols need to be subsetted during the call to be :ncol. I'm thinking about the scientist who adds a new variable to this list. They might or might not understand this nuance. When all arrays were being subsetted, the pattern was obvious. Now it is not so obvious ( but is more memory efficient). There is the added nuance of arrays going from ktop:pver (the new nlay). I suggest a comment to help the person modifying this call would be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a hopefully clarifying comment. Let me know if it could be improved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds great and hopefully will help folks in the future. "alread" needs the "y" added to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor typo, otherwise my review is complete
dtau_s(1:ncol,1:pver),dem_c(1:ncol,1:pver), & | ||
dem_s(1:ncol,1:pver),dtau_s_snow(1:ncol,1:pver), & | ||
dem_s_snow(1:ncol,1:pver),state%ps(1:ncol),cospstateIN,cospIN) | ||
call subsample_and_optics( & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds great and hopefully will help folks in the future. "alread" needs the "y" added to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great cleanup! I do have some questions and suggestions, but none of them are really required for this particular PR, so will go ahead and approve it now.
doc/ChangeLog
Outdated
the same vertical domain as the cloud parameterizations which is set the | ||
the namelist variable trop_cloud_top_press (1 mb by default). Changing to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo:
the same vertical domain as the cloud parameterizations which is set the | |
the namelist variable trop_cloud_top_press (1 mb by default). Changing to | |
the same vertical domain as the cloud parameterizations which is set by | |
the namelist variable trop_cloud_top_press (1 mb by default). Changing to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/physics/cam/ref_pres.F90
Outdated
'hPa', trop_pref, positive='down') | ||
|
||
allocate(trop_prefi(nlev+1), stat=istat) | ||
call alloc_err(istat, sub, 'trop_prefi', nlev) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be nlev+1
here?
call alloc_err(istat, sub, 'trop_prefi', nlev) | |
call alloc_err(istat, sub, 'trop_prefi', nlev+1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Fixed.
|
||
! The COSP init method was run from cospsimulator_intr_register in order to add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this comment is referring to the setcosp2values
call? If so then I might add it to the text here:
! The COSP init method was run from cospsimulator_intr_register in order to add | |
! The COSP init method (setcosp2values) was run from cospsimulator_intr_register in order to add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
'PARASOL-like mono-directional reflectance ', flag_xyfill=.true., fill_value=R_UNDEF) | ||
call addfld('CFAD_SR532_CAL', (/'cosp_sr','cosp_ht'/), 'A', 'fraction', & | ||
'Calipso Scattering Ratio CFAD (532 nm)', flag_xyfill=.true., fill_value=R_UNDEF) | ||
call addfld('MOL532_CAL', (/'trop_pref'/), 'A', 'm-1sr-1', & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might put a space here:
call addfld('MOL532_CAL', (/'trop_pref'/), 'A', 'm-1sr-1', & | |
call addfld('MOL532_CAL', (/'trop_pref'/), 'A', 'm-1 sr-1', & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
call addfld('CLD_CAL_TMP', (/'cosp_ht'/), 'A', 'percent', & | ||
'NOT SURE WHAT THIS IS Cloud Fraction', flag_xyfill=.true., fill_value=R_UNDEF) | ||
call addfld('CLD_CAL_TMPLIQ', (/'cosp_ht'/), 'A', 'percent', & | ||
'NOT SURE WHAT THIS IS Cloud Fraction', flag_xyfill=.true., fill_value=R_UNDEF) | ||
call addfld('CLD_CAL_TMPICE', (/'cosp_ht'/), 'A', 'percent', & | ||
'NOT SURE WHAT THIS IS Cloud Fraction', flag_xyfill=.true., fill_value=R_UNDEF) | ||
call addfld('CLD_CAL_TMPUN', (/'cosp_ht'/), 'A', 'percent', & | ||
'NOT SURE WHAT THIS IS Cloud Fraction', flag_xyfill=.true., fill_value=R_UNDEF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brianpm Any idea what these history fields are? We don't have to deal with these for this particular PR but it might be good to eventually either provide better long names or remove them depending on what these quantities are actually representing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are temperatures ("TMP").
CLD_CAL_TMP = Calipso Cloud Temperature
CLD_CAL_TMPLIQ = Calipso Liquid Cloud temperature
CLD_CAL_TMPICE = Calipso Ice Cloud temperature
CLD_CAL_TMPUN = Calipso Undefined-Phase Cloud Temperature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated field long names and units.
reff_cosp(1:ncol,1:pver,1:nhydro) = 0._r8 | ||
! note: reff_cosp dimensions should be same as cosp (reff_cosp has 9 hydrometeor dimension) | ||
! Reff(Npoints,Nlevels,N_HYDRO) | ||
|
||
use_precipitation_fluxes = .true. !!! consistent with cam4 implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you didn't write the code, but it looks like use_precipitation_fluxes
is always true, so why have it as a variable at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use_precipitation_fluxes
removed.
end do | ||
|
||
grpl_ls_interp = 0._r8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding it is that PUMAS microphysics is now capable of simulating graupel. Should there be an attempt to try and couple it to COSP here? This is probably yet another question for @brianpm (and doesn't have to be done for this particular PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. This is a good question, but I would need to look into whether the satellite products think they see graupel.
!! if use_reff=.false. then all sizes use DEFAULT_LIDAR_REFF = 30.0e-6 meters | ||
use_reff = .true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this variable actually being used? I can't seem to find its use anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use_reff
removed.
cospstateIN%lat = state%lat(:ncol)*180._r8/pi | ||
cospstateIN%lon = state%lon(:ncol)*180._r8/pi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could 180._r8/pi
be saved as a parameter instead of being re-calculated every call? I do realize this could cause round-off changes and so can certainly be ignored if answers are currently bit-for-bit for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Defined parameter rad2deg
.
call outfld('RH_COSP', cospstateIN%qv, ncol,lchnk) | ||
call outfld('Q_COSP', q(1:ncol,1:pver), ncol,lchnk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just double-checking that cospstateIN%qv
, which was originally set to be specific humidity, has been converted to what I assume is supposed to be relative humidity (RH_COSP
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see the conversion, but maybe it happens somewhere. I asked the same in my response about the names above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COSP takes specific humidity as an input. I replaced RH_COSP
(which was set to specific humidity) by Q_COSP
in the history output.
Merge pull request ESCOMP#1010 from brian-eaton/cosp-vlim cam6_4_005: Limit vertical domain used by COSP ESCOMP commit: 08d0326
Merge pull request ESCOMP#1010 from brian-eaton/cosp-vlim cam6_4_005: Limit vertical domain used by COSP ESCOMP commit: 08d0326
Merge pull request ESCOMP#1010 from brian-eaton/cosp-vlim cam6_4_005: Limit vertical domain used by COSP ESCOMP commit: 08d0326
The COSP simulator was not working with "FMT" compsets. This compset has a model top of about 1 Pa which is above where the cloud parameterizations operate. The COSP interface routine was modified so that COSP operates on the same vertical domain as the cloud parameterizations which is set the the namelist variable
trop_cloud_top_press
(1 mb by default). In addition a lot of code cleanup was done, and a bug fix was made for the layer interface values of height and pressure passed from CAM to COSP.resolves #967- COSP prevents running "FMT" compsets.
Removed old tools for topo file generation.
resolves #1005 - Remove old topo generation software from CAM