-
Notifications
You must be signed in to change notification settings - Fork 171
TUV-x photolysis #1471
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
base: cam_development
Are you sure you want to change the base?
TUV-x photolysis #1471
Conversation
modified: .gitmodules modified: cime_config/buildlib modified: libraries/tuv-x (new commits)
jimmielin
left a comment
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.
Thanks @fvitt - I had some minor suggested changes that are hopefully straightforward.
src/chemistry/mozart/chemistry.F90
Outdated
| call mee_ion_final() | ||
| call rate_diags_final() | ||
| call species_sums_final() | ||
| call tuvx_finalize() |
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 maybe wrap this within if (tuvx_active) to be consistent with the other calls
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
| !----------------------------------------------------------------- | ||
| ! ... get calculated photolysis rates from TUV-x | ||
| !----------------------------------------------------------------- | ||
| call tuvx_get_photo_rates( state, pbuf, ncol, lchnk, zmid, zint, & |
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 looks like the only place tuvx_get_photo_rates uses state is to get state%ncol in line ~679, but ncol is already passed in here. Maybe state does not have to be passed in to call tuvx_get_photo_rates
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.
fixed
| subroutine gas_phase_chemdr(lchnk, ncol, imozart, q, & | ||
| phis, zm, zi, calday, & | ||
| tfld, pmid, pdel, pint, rpdel, rpdeldry, & | ||
| cldw, troplev, troplevchem, & | ||
| ncldwtr, ufld, vfld, & | ||
| delt, ps, & | ||
| fsds, ts, asdir, ocnfrac, icefrac, & | ||
| precc, precl, snowhland, ghg_chem, latmapback, & | ||
| drydepflx, wetdepflx, cflx, fire_sflx, fire_ztop, nhx_nitrogen_flx, noy_nitrogen_flx, & | ||
| use_hemco, qtend, pbuf) | ||
| drydepflx, wetdepflx, cflx, fire_sflx, fire_ztop, & | ||
| nhx_nitrogen_flx, noy_nitrogen_flx, & | ||
| use_hemco, qtend, pbuf, state) |
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 changes appear to be unnecessary as state was already previously passed in as the first argument, and was moved to the last one for some reason.
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.
fixed
| use mo_chm_diags, only : chm_diags, het_diags | ||
| use perf_mod, only : t_startf, t_stopf | ||
| use gas_wetdep_opts, only : gas_wetdep_method | ||
| use physics_types, only : physics_state |
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.
This line is a duplicate of line 318 and does not need to be added again
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.
The duplicate line has been removed
src/chemistry/mozart/chemistry.F90
Outdated
| call gas_phase_chemdr(lchnk, ncol, imozart, state%q, & | ||
| state%phis, state%zm, state%zi, calday, & | ||
| state%t, state%pmid, state%pdel, state%pint, state%rpdel, state%rpdeldry, & | ||
| cldw, tropLev, tropLevChem, ncldwtr, state%u, state%v, chem_dt, state%ps, & | ||
| fsds, cam_in%ts, cam_in%asdir, cam_in%ocnfrac, cam_in%icefrac, & | ||
| cam_out%precc, cam_out%precl, cam_in%snowhland, ghg_chem, state%latmapback, & | ||
| drydepflx, wetdepflx, cam_in%cflx, cam_in%fireflx, cam_in%fireztop, & | ||
| nhx_nitrogen_flx, noy_nitrogen_flx, use_hemco, ptend%q, pbuf ) | ||
| nhx_nitrogen_flx, noy_nitrogen_flx, use_hemco, ptend%q, pbuf, state ) |
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 changes appear to be unnecessary as state was already previously passed in as the first argument, and was moved to the last one for some reason.
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/chemistry/mozart/mo_tuvx.F90
Outdated
| integer, parameter :: PROFILE_INDEX_SO2 = 7 ! Sulfur dioxide profile index | ||
| integer, parameter :: PROFILE_INDEX_NO2 = 8 ! Nitrogen dioxide profile index |
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 appear to be non-public and unused, can I ask if these are intended to be not implemented?
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.
removed
src/chemistry/mozart/mo_tuvx.F90
Outdated
| ! Definition of the MS93 wavelength grid TODO add description of this | ||
| integer, parameter :: NUM_BINS_MS93 = 4 | ||
| real(kind=r8), parameter :: WAVELENGTH_EDGES_MS93(NUM_BINS_MS93+1) = & | ||
| (/ 181.6_r8, 183.1_r8, 184.6_r8, 190.2_r8, 192.5_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.
Missed TODO here about the description here
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.
added a brief description
src/chemistry/mozart/mo_tuvx.F90
Outdated
| deallocate( cam_grids ) | ||
| deallocate( cam_profiles ) | ||
| deallocate( cam_radiators ) | ||
| deallocate( wavelength ) |
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.
Since wavelength => cam_grids%get_grid(...) I would suggest switching the deallocate order for safety
| deallocate( cam_grids ) | |
| deallocate( cam_profiles ) | |
| deallocate( cam_radiators ) | |
| deallocate( wavelength ) | |
| deallocate( wavelength ) | |
| deallocate( cam_grids ) | |
| deallocate( cam_profiles ) | |
| deallocate( cam_radiators ) |
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
|
|
||
| host_profile => profiles%get_profile( "temperature", "K" ) | ||
| this%profiles_( PROFILE_INDEX_TEMPERATURE ) = this%core_%get_updater( host_profile, found ) | ||
| call assert( 418735162, found ) |
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.
Here and throughout the file, there are many calls to musica_assert::assert with seemingly random numbers for unique error codes. It seems to be consistent with what's done elsewhere in MUSICA, but I would request to at least have a comment somewhere that these numbers are just unique error codes and don't have any particular meaning other than being unique, and this is how the MUSICA library handles errors. Thanks!
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.
added comments
src/chemistry/mozart/mo_tuvx.F90
Outdated
| ! || || | ||
| ! || | ||
| ! ----------------- i_int = pver | ||
| ! ================= i_imd = pver ------------------ i_int = 2 |
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 here:
| ! ================= i_imd = pver ------------------ i_int = 2 | |
| ! ================= i_mid = pver ------------------ i_int = 2 |
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.
fixed
deleted: src/chemistry/mozart/mo_setz.F90 deleted: src/chemistry/mozart/mo_tuv_inti.F90 modified: src/chemistry/mozart/chemistry.F90 modified: src/chemistry/mozart/mo_gas_phase_chemdr.F90 modified: src/chemistry/mozart/mo_tuvx.F90
nusbaume
left a comment
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.
Thanks for bringing this in @fvitt! I had some cleanup requests in the python code, but otherwise only had a couple other comments. Hopefully none of my requests will cause any issues, but if you find that they do please let me know!
cime_config/testdefs/testmods_dirs/cam/carma_mixed_sulfate/user_nl_cam
Outdated
Show resolved
Hide resolved
cime_config/testdefs/testmods_dirs/cam/outfrq9s_waccm_ma_mam4/user_nl_cam
Show resolved
Hide resolved
modified: cime_config/buildlib modified: cime_config/buildnml modified: cime_config/testdefs/testmods_dirs/cam/carma_mixed_sulfate/user_nl_cam
cacraigucar
left a comment
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 would have expected an aux_cam regression test. Can you modify an existing test to turn this on (or is there one already)?
| [submodule "tuv-x"] | ||
| path = libraries/tuv-x | ||
| url = https://github.com/NCAR/tuv-x.git | ||
| fxtag = ecf768a2b66 |
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.
We need a tag for this instance
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.
@K20shores, Can you make a new tag?
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.
Created: https://github.com/NCAR/tuv-x/tree/v0.14.1
tag: v0.14.1
modified: src/chemistry/mozart/chemistry.F90 modified: src/chemistry/mozart/mo_jeuv.F90
| call assert_msg( 973680295, & ! random error number consistent with how MUSICA error handling | ||
| cam_config%validate( required_keys, optional_keys ), & | ||
| "Bad configuration for CAM TUV-x options." ) |
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 curious why this random number approach is being used instead of the __FILE__and __LINE__ which is being used elsewhere in CESM is not being used. That approach leads the user to the exact spot without needing to do a grep or table lookup.
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.
This style of error reporting was implemented by @mattldawson. If we are not happy with this style in CAM code, I propose we use endrun to abort.
@cacraigucar Let me know if these asserts need to be replaced.
|
TUV-x is turned on in this aux_cam test: See changes here: |
nusbaume
left a comment
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.
Thanks for resolving my concerns @fvitt!
However, looking over the code again I realized that there might be no need to create a build variable in the CMake python functions, as I believe everything can just use case.get_value(), so I have a couple remaining change requests related to that (along with a typo fix). Otherwise everything else looks good!
| ubc_specifier = 'T->MSIS', 'Q->2.d-8vmr', 'CH4->2.d-10vmr', 'H->MSIS', 'N->MSIS', 'O->MSIS', 'O2->MSIS', 'H2->TGCM', 'NO->SNOE' | ||
| tuvx_active = .true. | ||
| tuvx_config_path = '$SRCROOT/src/chemistry/pp_waccm_ma_sulfur/tuvx_config.json' | ||
| tuvx_config_path = '$COMPCOMP_ROOT_DIR_ATM/src/chemistry/pp_waccm_ma_sulfur/tuvx_config.json' |
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 here (COMPCOMP -> COMP).
| args += "-DOS={} ".format(build.get_value("OS")) | ||
| args += "-DMACH={} ".format(case.get_value("MACH")) | ||
| args += f"-DCASEROOT={caseroot} " | ||
| args += f"-DCOMPILER={build.get_value('COMPILER')} " |
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.
Apologies for not catching this in the first review round, but I think you can just use the case.get_value method here and below, in which case you can remove the build = EnvBuild(case_root=caseroot) line above.
| # Builds the TUV-x library and updates the case variables used to set the | ||
| # include paths and linked libraries | ||
|
|
||
| build = EnvBuild(case_root=caseroot) |
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 think you can use case.get_value instead of build.get_value for all of the uses below, in which case I don't believe you need this line anymore.
This provides the option to use the TUV-x library to compute photo-chemical rates in-line as an alternative to the traditional table look up method.
Closes #757