-
Notifications
You must be signed in to change notification settings - Fork 0
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
Cloud chemistry clean-up #10
Cloud chemistry clean-up #10
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 like what you've done. It helps me understand the code better. I have a few suggestions and questions, hopefully the changes I'm suggesting aren't too time-consuming to make.
I'll check which comments in the #5 review thread are being addressed by your PR and leave comments there tomorrow.
I’ve left comments on the review comments (#5) that seem to be resolved by this PR. If people agree, we can click |
Oh, I went ahead and did that. I trust your judgement |
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.
Thank you for addressing the comments!
endif | ||
|
||
if ( .not. cloud_borne) then ! this seems to be specific to aerosols that are not cloud borne | ||
xh2o2(i,k) = xh2o2(i,k) + r2h2o2*dtime ! updated h2o2 by het production | ||
xh2o2(i,k) = xh2o2(i,k) + r2h2o2*time_step ! updated h2o2 by het production |
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.
@mattldawson I am just curious, sometimes you use r2XXX (which is what the original code does) like this one and sometimes you use dXXX_dt like dso4_dt in line 731. Say, do you want r2XXX for gases and dXXX_dt for aerosols? Not asking for code changes, just curious if there is any rationale for the distinction. 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.
No, this was probably just an oversight - I'll update the variable name.
ccc = pso4*dtime | ||
ccc = max(ccc, 1.e-30_r8) | ||
delta_concentration = dso4_dt*time_step | ||
delta_concentration = max(delta_concentration, 1.e-30_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.
@mattldawson Hmm, I wonder if you want to define a variable for this 1.e-30. But this is very optional
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 was struggling with this, and forgot to add a comment here. I would have called this something like SMALL_NUMBER
but then there are other "small numbers" with different values, like here. Maybe these could all be the same small number, but I wasn't sure.
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.
Yea, I don't know why they are different - I think those small values are a little bit arbitrarily added, not sure. Perhaps those people tested the code and found out different numbers worked better in different situations. Maybe we can try to assign the same small value here for all concentrations (1e-30). I think if we test the new code and the results look similar to the original code, then we can use one single small number for all concentrations, no matter gas phase or aerosol concentrations.
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 few small comments but the code looks good to me, thanks Matt! Feel free to tag me if anywhere in the code needs further particular attention.
c58c42e
into
develop-1-review-cloud-chemistry
Updates the common code for calculating cloud aqueous chemistry.
cloud_species_t
class for shared index lookup logicpartially satisfies #7
@dmleung @K20shores @boulderdaze - could you check if these changes address any of your open comments on #5? If so, we can close them when this gets merged into the cloud review branch.