-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add cloud optical calculations for use in TUV-x #167
Add cloud optical calculations for use in TUV-x #167
Conversation
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.
Looks good! I only have a couple of minor suggestions
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.
Looks good! Thank you for addressing the comments!
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 @mattldawson! I have a couple questions regarding the use of ccpp_const_utils
but otherwise the code looks great.
@@ -13,13 +13,13 @@ subroutine ccpp_const_get_idx(constituent_props, name, cindex, errmsg, errflg) | |||
use ccpp_constituent_prop_mod, only: ccpp_constituent_prop_ptr_t | |||
|
|||
! Input arguments | |||
type(ccpp_constituent_prop_ptr_t), pointer, intent(in) :: constituent_props(:) | |||
character(len=*), intent(in) :: name ! constituent name | |||
type(ccpp_constituent_prop_ptr_t), intent(in) :: constituent_props(:) |
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 I ask if there's any particular reason the pointer
attribute was removed from here?
ccpp_const_get_idx
is used in cam_constituents
as well so this change would affect existing code - could you confirm that CAM-SIMA still passes tests after removing the pointer
attribute here? I think it is fine if the actual argument is a pointer
(in cam_constituents::const_props
) but the dummy argument is not, as long as it's read only, but it would be good to check...
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 pointing this out, I had meant to add a comment about this, but I forgot when I submitted the PR.
I removed the pointer
attribute because we don't include this attribute for constituent_props
in the musica module functions, and it isn't actually needed in the ccpp_const_get_idx
function, as you point out.
However, if this function is being used through an interface (e.g., for a function pointer or an implementation of a deferred type-bound procedure) where the pointer
attribute couldn't be removed for some reason, we could do a separate implementation of the ccpp_const_get_idx
function for non-pointers. Given the intent of this function, it seemed pretty unlikely to me that this was the case, but I agree it's good to check. Maybe @peverwhee could weigh in?
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 @mattldawson! If it isn't actually needed (should be fine if it's intent(in)
) I'm happy to accept the change, just wanted to make sure it wouldn't break anything else. I'll leave this conversation open in case @peverwhee would like to add comments but I'll approve the PR now - no need to wait for me!
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'm ok with removing the pointer attribute as well - I just ran the regression tests with it removed and they work fine so I'll agree with @jimmielin that it isn't needed.
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.
awesome, thanks!
Tag name (The PR title should also include the tag name): `atmos_phys0_07_001` Originator(s): @jimmielin List all `development` PR URLs included in this PR and a short description of each: * Update extraterrestrial flux in TUV-x prior to calculating rate constants #152 by @boulderdaze * Simplify deallocation of multiple objects associated with the TUV-x #156 by @boulderdaze * Fill in errmsg, errflg in check_energy schemes #160 by @jimmielin * Validates the MUSICA meta data against the CCPP standard names #162 by @boulderdaze * Add constituent tendency updater #111 by @peverwhee * Add cloud optical calculations for use in TUV-x #167 by @mattldawson * Add initialize_constituents scheme #149 by @peverwhee * Add diagnostics to TJ2016 test schemes #170 by @peverwhee * update "radians" to "rad" #173 by @peverwhee * Solar zenith angle and Earth-Sun distance #171 by @mattldawson * Update standard names for tropopause_find #140 by @jimmielin * Update surface albedo units #181 by @mattldawson * don't set water species property for species that air_composition handles #185 by @peverwhee List all test failures: N/A
Adds the calculation of cloud optical properties to TUV-x for photolysis rate constant calculations. The algorithm is adapted from a post-calculation modification of photolysis rate constants calculated by the lookup version of TUV-x in CAM to account for the effects of clouds, here
closes #100
Adds a few more Valgrind suppressions for minor memory leaks during initialization functions in 2 tests (9 and 72 bytes). We will look into these, but they are expected to go away as TUV-x is moved out of Fortran.