Skip to content

Conversation

@chihchen24
Copy link
Collaborator

@chihchen24 chihchen24 commented Nov 17, 2020

closes #274

contrail parameterization added to CAM

@andrewgettelman @cacraigucar

@cacraigucar cacraigucar self-assigned this Dec 10, 2020
Copy link
Collaborator

@cacraigucar cacraigucar left a comment

Choose a reason for hiding this comment

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

Partial review (not completed yet). Submitting this as I know @chihchen24 is working hours offset from mine.

@gold2718 and @nusbaume - Can you please review my comments and augment them as needed. I've highlighted the ones which I'd like your feedback on. There are a few sections where I'm not sure if my proposed solution is the best one or if you have a better solution. I don't want to have @chihchen24 implement a change only to have to change it again after full code review.

Note that I know there are additional changes coming, but I am submitting this before I put it down for the day.

@cacraigucar cacraigucar added this to the CESM2.3 milestone Jan 6, 2021
Copy link
Collaborator

@cacraigucar cacraigucar left a comment

Choose a reason for hiding this comment

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

Finished second phase of my preliminary review

Copy link
Collaborator

@cacraigucar cacraigucar left a comment

Choose a reason for hiding this comment

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

It is getting much closer!

Copy link
Collaborator

@cacraigucar cacraigucar left a comment

Choose a reason for hiding this comment

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

Please make sure you compile and run your test on this code before requesting a review again as I found a typo that would prohibit the code from compiling.

@cacraigucar cacraigucar marked this pull request as draft February 25, 2021 16:42
@cacraigucar
Copy link
Collaborator

@chihchen24 I will need to get from you a testing setup that tests the contrail setup. Could you please give me a create_newcase and any namelist mods that you are using to run this.

@chihchen24
Copy link
Collaborator Author

chihchen24 commented Mar 2, 2021 via email

@cacraigucar cacraigucar requested a review from fvitt March 3, 2021 22:40
@chihchen24 chihchen24 requested a review from gold2718 March 9, 2021 05:08
Copy link
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

There are still a couple of unused imports (geopotential_dse, pbuf_set_field, get_curr_date).
There are still some old fashioned logical tests (e..g, .gt.) in unmodified parts of the file, please search and fix.

@chihchen24 chihchen24 requested a review from gold2718 March 9, 2021 05:50
Copy link
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing all those old lines.
Looks good now, thanks!

@peverwhee peverwhee removed their request for review March 9, 2021 09:02
@cacraigucar
Copy link
Collaborator

@fvitt and @nusbaume Could you please check your open conversations and indicate whether or not you are happy with the changes. We are still awaiting final approval from both of you.

@cacraigucar
Copy link
Collaborator

@chihchen24 We still need to setup a regression test for this configuration. I believe we still need the monthly files (complete with required AMWG metadata). Once you supply the list to me I can import them into the svn repo. We will use these files for a simple nine time step test to add to our regression suite.

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 now, thanks! I do have one minor change, but otherwise I approve.

Mh2o = mwh2o

! contrail paramter (G = CF*p/epi)
! and Schumann 1996 DOI: 10.1127/metz/5/1996/4, reprinted by Ponater 2002, JGR (eq 6-8) DOI: 10.1029/2011MS000105
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the DOI for Ponater 2002 here is incorrect, and instead should be:

DOI: 10.1029/2001JD000429

Copy link
Collaborator

Choose a reason for hiding this comment

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

That DOI is correct.

@cacraigucar
Copy link
Collaborator

@siouxyalie We are adding a new contrail parameterization. Do we need to add anything to keep your horizontal momentum tendency current? If so, could you please supply the suggested code modifications?

@lignumvit
Copy link

@cacraigucar After a quick skim, I don't think U or V are modified by this parameterization, so the additions in this PR should not affect the momentum budget closure. If you all think that this parameterization might one day affect U and V, then some lines to add and output ?TEND_CONTRAIL would have to be added. However, I'd guess that parameterizations of this process will never end up modifying U and V. So, no edits suggested.

@andrewgettelman
Copy link
Collaborator

U & V are not going to be modified by this parameterization

@cacraigucar cacraigucar requested a review from gold2718 March 17, 2021 23:11
@cacraigucar
Copy link
Collaborator

@gold2718 Could you please review the regression test? @chihchen24 Could you please checkout and run the current code one more time to make sure you are happy with it (your _v5 run used an older version of the code).

@chihchen24
Copy link
Collaborator Author

chihchen24 commented Mar 18, 2021

@cacraigucar I have checked out the current code and did a test run. It went fine. Thank you.

@cacraigucar cacraigucar changed the title contrail parameterization added cam6_3_014?: contrail parameterization added Mar 18, 2021
Copy link
Collaborator

@gold2718 gold2718 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!

miscellaneous updates & fixes, including fixing cam_snapshot with CLUBB and nuopc cap changes
@cacraigucar cacraigucar changed the title cam6_3_014?: contrail parameterization added cam6_3_014: contrail parameterization added Mar 19, 2021
miscellaneous updates & fixes, including fixing cam_snapshot with CLUBB and nuopc cap changes
@cacraigucar cacraigucar merged commit 4e152b7 into ESCOMP:cam_development Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants