Skip to content
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_132: Changes to capture changes made in run 51 #900

Merged
merged 14 commits into from
Oct 18, 2023

Conversation

cacraigucar
Copy link
Collaborator

Closes #895

@cacraigucar cacraigucar self-assigned this Oct 10, 2023
@cacraigucar cacraigucar marked this pull request as draft October 10, 2023 20:47
Copy link

@adamrher adamrher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one concern about using cam_dev as the logical for the new ice fraction mods.

src/physics/cam/cldfrc2m.F90 Outdated Show resolved Hide resolved
src/physics/cam/cldfrc2m.F90 Outdated Show resolved Hide resolved
@PeterHjortLauritzen
Copy link
Collaborator

PeterHjortLauritzen commented Oct 12, 2023

I have one concern about using cam_dev as the logical for the new ice fraction mods.

Agreed (the initial implementation of do_hb_above_clubb used cam_physpkg_is("cam_dev") as a specifier but I changed it to namelist for the PR).

@adamrher
Copy link

Agreed (the initial implementation of do_hb_above_clubb used cam_physpkg_is("cam_dev") as a specifier but I changed it to namelist for the PR).

OK, I am going to make a namelist called cldfrc2m_do_avg_aist_algs since what these mods do is average two different aist algorithms. Will do this after my 1PM mtg.

Copy link
Collaborator

@PeterHjortLauritzen PeterHjortLauritzen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor namelist comment.

Thanks @adamrher and @cacraigucar

@@ -2065,9 +2065,11 @@
<clubb_do_energyfix > .true. </clubb_do_energyfix>
<clubb_do_liqsupersat > .false. </clubb_do_liqsupersat>
<clubb_gamma_coef > 0.308 </clubb_gamma_coef>
<clubb_gamma_coef phys="cam_dev" > 0.3 </clubb_gamma_coef>
<clubb_gamma_coef hgrid="1.9x2.5" phys="cam6" > 0.280 </clubb_gamma_coef>
<clubb_gamma_coef dyn="se" > 0.270 </clubb_gamma_coef>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove

<clubb_gamma_coef dyn="se" > 0.270 </clubb_gamma_coef>

?

Copy link

@adamrher adamrher Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good catch actually because it results in a tie if you run w/ cam_dev and se. We can either do away with it entirely, or specify it further to refer to how se is configured for cam6 in the CESM2.2 JAMES paper:

<clubb_gamma_coef dyn="se" phys="cam6"     > 0.270   </clubb_gamma_coef>

Copy link
Collaborator Author

@cacraigucar cacraigucar Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd seen that and confirmed that the first one appeared to beat out the later one. Can we just delete the <clubb_gamma_coef dyn="se" phys="cam6" > 0.270 </clubb_gamma_coef> as it is captured in the CESM2.2 branch for archival purposes?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Yes, I concur with deleting it entirely from cam_development.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is resolved in PR pending to this branch. Note that removing this line will cause a lot of baseline failures.

adamrher and others added 2 commits October 12, 2023 14:51
swap cam_dev conditional for a new namelist, remove se gamma namelist…
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just have some minor variable comment/description cleanup.

bld/namelist_files/namelist_definition.xml Outdated Show resolved Hide resolved
bld/namelist_files/namelist_definition.xml Outdated Show resolved Hide resolved
src/physics/cam/cldfrc2m.F90 Show resolved Hide resolved
src/physics/cam/cldfrc2m.F90 Outdated Show resolved Hide resolved
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good to me now, thanks!

@cacraigucar
Copy link
Collaborator Author

@adamrher - Here is the fix I implemented for the failing SPCAM regression test. a35cef1
The test now passes with this fix. Let me know if you approve of the change.

@adamrher
Copy link

Looks good to me @cacraigucar. I might just add a comment explaining that wsub_min_asf is:

! minimum sub-grid vertical velocity

@cacraigucar cacraigucar changed the title Changes to capture changes made in run 51 cam6_3_132: Changes to capture changes made in run 51 Oct 16, 2023
@cacraigucar
Copy link
Collaborator Author

Answer change mystery solved (by @fischer-ncar). There is a CMEPS update which is answer changing. It is because of ESCOMP/CMEPS#394. This change was signed off on by @PeterHjortLauritzen a little over a month ago. The upcoming CESM tag will defintely have this. Is it okay to have the CAM tag have this answer change as well at this time?

If so, my plan is to proceed with making cam6_3_131 with the updated externals and then rereunning the regression tests for this PR. The answer changes should be limited to cam_dev at this point.

Thoughts?
@cecilehannay @adamrher @PeterHjortLauritzen

@adamrher
Copy link

@cacraigucar that sounds like a good plan. Fingers crossed that most non-cam_dev baselines don't fail when you do the new regression tests for the 51 changes.

@cacraigucar
Copy link
Collaborator Author

@cacraigucar that sounds like a good plan. Fingers crossed that most non-cam_dev baselines don't fail when you do the new regression tests for the 51 changes.

Based on @adamrher approval of my workflow and @PeterHjortLauritzen's previous approval, I am proceeding to make cam6_3_131 with the updated externals. I will then startup regression testing on this PR.

@cecilehannay
Copy link
Collaborator

It sounds good. We also talked at the CESM meeting about creating a new CESM tag using this new cam tag + any changes to MOM the oceanographers want. (@gustavo-marques, @jedwards4b)

@PeterHjortLauritzen
Copy link
Collaborator

Sounds good to include the CMEPS change! Approved ...

@cacraigucar cacraigucar merged commit 5ddccd8 into ESCOMP:cam_development Oct 18, 2023
@cacraigucar cacraigucar deleted the cam_match_run_51 branch October 18, 2023 15:42
gold2718 pushed a commit to gold2718/CAM that referenced this pull request May 2, 2024
Merge pull request ESCOMP#900 from cacraigucar/cam_match_run_51

cam6_3_132: Changes to capture changes made in run 51

ESCOMP commit: 5ddccd8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Tag
Development

Successfully merging this pull request may close these issues.

5 participants