-
Notifications
You must be signed in to change notification settings - Fork 133
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_3_006: initial implementation of CLUBB+MF #221
cam6_3_006: initial implementation of CLUBB+MF #221
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.
Pardon the piecemeal nature of my comments, still figuring out the workflow. Almost all of my comments address comment typos I let slide when I inherited this code, but now that others are using this I'd like to see them corrected.
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.
Once these changes have been made and pushed back to your PR, please ask for a re-review (hit the two circular arrows by my reviewer name). At that point, I will add the other CAM-SE reviewers to the list for their review.
src/physics/cam/edmf_module.F90
Outdated
sigmaw = 0.572_r8 * wstar / 1._r8 | ||
sigmaqt = 2.890_r8 * abs(qstar) / 1._r8 | ||
sigmath = 2.890_r8 * abs(thstar)/ 1._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.
We like to document constants. What are 0.572_r8 and 2.890_r8? Are these from a paper or do they have a definition?
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.
made these constants and added descr. + reference.
src/physics/cam/edmf_module.F90
Outdated
upqt(1,i) = qt(1) + 0.32_r8 * upw(1,i) * sigmaqt/sigmaw | ||
upthv(1,i) = thv(1) + 0.58_r8 * upw(1,i) * sigmath/sigmaw |
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.
More constants to possibly define
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.
made these constants and added descr. + reference.
Hi Cheryl, I've made most of the changes you had asked for, but I need a morning and some fresh coffee to make sure I'm ready to push my changes. |
@cacraigucar it is saying I have a conflict with my new ChangeLog, but I can't see anything wrong. Any idea what's going on here? EDIT: ohh. I presume it's because the head of CAM is cam6_3_001 and I'm branching off of cam6_3_000. Seems like an easy fix, but I can't hit the resolve conflicts button. Should I add in the cam6_3_001 ChangLog entry in my branch and commit? |
We'll worry about the ChangeLog when we get ready to commit. For now, don't worry about it. |
yep! |
FYI - the merge broke the regression test, due to changes with PUMAS. Fix will be coming after lunch. |
The issue was that I forgot to rerun checkout_externals. The restart test now runs fine. @adamrher - The answers do change due to the PUMAS update (and maybe other updates as well). Please rerun and make sure you are happy with the results. I will continue with my testing. |
@cacraigucar what source code do I test? I don't see any more commits in my branch. |
@adamher - I committed it to ESCOMP/CAM (upstream) instead of your branch! I'll be backing it out of ESCOMP/CAM and committing it to your branch. New workflow for me and I blew it! Look for the commit message shortly (first undoing ESCOMP/CAM commits) |
After further investigation, it was concluded that the commit was actually done correctly. There will be another minor commit coming soon. |
@cacraigucar results of SCAM runs look good to me with the cam6_3_005 merge onto my branch |
test/system/test_driver.sh
Outdated
# Run git and r8 tests | ||
export ADDREALKIND_EXE=/fs/cgd/csm/tools/addrealkind/addrealkind; ${CAM_ROOT}/test/system/TR8.sh | ||
${CAM_ROOT}/test/system/TGIT.sh |
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.
Does this run the git and r8 tests when test_driver.sh is invoked from the command line on izumi? How long does the r8 test take to run? Does the output get written to a log file?
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 results are written to the screen only and run for both PGI and NAG izumi runs. The r8 tests take less than a minute and TGIT is instantaneous. In the commit that I made an hour or so ago, there are now lines around the runs to help the user to see the results.
src/physics/cam/clubb_mf.F90
Outdated
!if (debug) then | ||
! if ( masterproc ) then | ||
! write(iulog,*) "B(k,i), k, i ", B, k, i | ||
! end if | ||
!end if |
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 this output is off by default (controlled by debug
), either remove it or uncomment 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.
@mikaelwitte do you think we should uncomment this for this PR? It makes the log files massive, but other than that, I think we agreed earlier it we good to have at least one debug print statement.
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 it's worth leaving it in as a sanity check during plume microphysics development.
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 like some commented debug code removed but no need to re-review after that.
Note that there are unresolved conversations started by @nusbaume and @cacraigucar, please check that your concern has been addressed and resolve each conversation (or let us know if there is still an issue).
@gold2718 and @adamrher The added file in namelist_defaults.xml appears to be the reason for the answer change. By removing it, the baseline tests pass. Since it was thought this file was not being used, I will run some additional tests to confirm this, but I wanted to post this information as soon as I could (after izumi hiccuped on building this test overnight!) |
@fvitt Have you completed your review or are you still working on it? |
Code mods are limited to CAM. No externals needed to be modified, contrary to the description in the original issue. I've populated the ChangeLog with all the relevant information that I could think of.
The mods do not impact the answers because the mass fluxes default to off. I would like to develop a test to protect this functionality with the mass fluxes on, and suggest the following configuration
--comp FSCAM
--res T42_T42
--user-mods-dir $src/cime_config/usermods_dirs/scam_bomex
do_clubb_mf = .true.
deep_scheme='off'
Fixes #181
Code mods also fix issue with scam (this is why number of files changed is so inflated)
Fixes #213
Fixes #281
@JulioTBacmeister @mikaelwitte