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_4_006: fix CLUBB interface bug #1054

Merged

Conversation

PeterHjortLauritzen
Copy link
Collaborator

fixes #1053

@PeterHjortLauritzen PeterHjortLauritzen added the bug-fix This PR was created to fix a specific bug. label Jun 4, 2024
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious why make this fix in place here instead of fixing set_wet_to_dry in physics_types.F90?
The "broken" set_wet_to_dry is still called in vertical_diffusion_tend which is called in tphysac.

Copy link
Collaborator Author

@PeterHjortLauritzen PeterHjortLauritzen Jun 6, 2024

Choose a reason for hiding this comment

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

What the subroutine set_wet_to_dry does would be clearer if it was named undo_set_dry_to_wet or revert_set_dry_to_wet

Explanation:

subroutine set_dry_to_wet does as the subroutine name suggests: it converts mixing ratios that are dry ((cnst_type(m).eq.'dry')) to wet.

subroutine set_wet_to_dry reverts this operation by converting the tracers that are supposed to be dry back to dry (from wet).

set_wet_to_dry does NOT convert wet tracers to dry as the name seems to imply: here is the code from set_wet_to_dry

  do m = 1,pcnst
     if (cnst_type(m).eq.'dry') then
        state%q(:ncol,:,m) = state%q(:ncol,:,m)*state%pdel(:ncol,:)/state%pdeldry(:ncol,:)
     endif
  end do

Hence the operation in vertical_diffusion_tend is correct as it converts dry tracers to wet and reverts the operation at the end.

Converting all of CAM physics to a dry basis would highly likely make sure these kinds of bugs are avoided ... and vertical diffusion would not create spurious gradients for constant dry mixing ratios ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, thanks. I keep forgetting about that subtlety (probably because of the broken name and also getting rusty in that part of the code). Also, there is no tag on a constituent's current state which has caused plenty of other problems in the past. I'm still hoping this can get fixed in CAM-SIMA but I don't know if that has been fully fleshed out yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@PeterHjortLauritzen @gold2718 I have wondered this question for a while: why do we need to use dry basis and wet basis in different CAM parts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Traditionally, moist physics developers have always used moist mixing ratios for water species. For example, specific humidity (which equals the moist mixing ratio of water vapor) is a directly measurable and observable quantity, making it a logical choice for moist physics. However, chemists usually use dry mixing ratios; for them, a moist basis is problematic because it includes a water vapor signal embedded in the mixing ratio. Numerical schemes are generally designed to conserve a constant mixing ratio. Specifically, some chemical species remain constant (on a dry basis) in most of the atmosphere. If one uses a moist basis, the numerical schemes will likely introduce a water vapor signal into the mixing ratio that was supposed to remain constant.

We wrote a detailed argument for using a dry basis, which you can find here:

https://agupubs.onlinelibrary.wiley.com/doi/epdf/10.1029/2017MS001257

(see Introduction)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @PeterHjortLauritzen so much for your detailed explanation and sharing of your paper. That is very helpful.

Copy link
Collaborator

@brian-eaton brian-eaton left a comment

Choose a reason for hiding this comment

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

I agree with Steve's initial reaction: why don't we fix the utility code in physics_types.F90? By doing so we have the opportunity to add some clarity the complexity of having different types of constituents and different parameterizations that expect wet or dry mixing ratios based on the type of the constituent (water specie or not). I have made an attempt at this below and invite comments for further clarification. I left the subroutine names the same since the functionality of converting between wet and dry mixing ratios is fairly clear in those names. What's not clear is what constituent types the conversion is applied to. Adding an argument will make that clearer. I considered making the argument optional with the default to convert the dry species as it currently does. But there aren't many calls to these subroutines so updating them all is easy, and will make existing code more clear.

subroutine set_wet_to_dry(state, convert_cnst_type)

  ! Convert mixing ratios from a wet to dry basis for constituents of type
  ! convert_cnst_type.  Constituents are given a type when they are added
  ! to the constituent array by a call to cnst_add during the register
  ! phase of initialization.  There are two constituent types: 'wet' for
  ! water species and 'dry' for non-water species.

  use constituents,  only: pcnst, cnst_type

  type(physics_state),  intent(inout) :: state
  character(len=*),     intent(in)    :: convert_cnst_type
  
  ! local variables
  integer :: m, ncol
  character(len=*), parameter :: sub = 'set_wet_to_dry'
  !-----------------------------------------------------------------------------

  ! check input
  if (.not.(convert_cnst_type == 'wet' .or. convert_cnst_type == 'dry')) then
    write(iulog,*) sub//': FATAL: convert_cnst_type not recognozed: '//convert_cnst_type
    call endrun(sub//': FATAL: convert_cnst_type not recognozed: '//convert_cnst_type)
  end if

  ncol = state%ncol

  do m = 1,pcnst
     if (cnst_type(m) .eq. convert_cnst_type) then
        state%q(:ncol,:,m) = state%q(:ncol,:,m)*state%pdel(:ncol,:)/state%pdeldry(:ncol,:)
     endif
  end do

end subroutine set_wet_to_dry

Similar changes can be made to set_dry_to_wet

@PeterHjortLauritzen
Copy link
Collaborator Author

Thanks for looking into this. I am fine with the proposed change.

@brian-eaton brian-eaton self-requested a review June 24, 2024 18:29
Copy link
Collaborator

@brian-eaton brian-eaton 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 implemented the suggested changes.

@brian-eaton brian-eaton requested a review from cacraigucar June 24, 2024 18:32
@cacraigucar cacraigucar requested review from nusbaume and removed request for cacraigucar June 24, 2024 18:36
@cacraigucar cacraigucar added the answer changing answer changing tag label Jun 25, 2024
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 to me! Only issue I found was few minor typos (so I don't feel the need to re-review). Thanks!

Comment on lines 1504 to 1505
write(iulog,*) sub//': FATAL: convert_cnst_type not recognozed: '//convert_cnst_type
call endrun(sub//': FATAL: convert_cnst_type not recognozed: '//convert_cnst_type)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couple minor typos:

Suggested change
write(iulog,*) sub//': FATAL: convert_cnst_type not recognozed: '//convert_cnst_type
call endrun(sub//': FATAL: convert_cnst_type not recognozed: '//convert_cnst_type)
write(iulog,*) sub//': FATAL: convert_cnst_type not recognized: '//convert_cnst_type
call endrun(sub//': FATAL: convert_cnst_type not recognized: '//convert_cnst_type)

Comment on lines 1539 to 1542
if (.not.(convert_cnst_type == 'wet' .or. convert_cnst_type == 'dry')) then
write(iulog,*) sub//': FATAL: convert_cnst_type not recognozed: '//convert_cnst_type
call endrun(sub//': FATAL: convert_cnst_type not recognozed: '//convert_cnst_type)
end if
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typos:

Suggested change
if (.not.(convert_cnst_type == 'wet' .or. convert_cnst_type == 'dry')) then
write(iulog,*) sub//': FATAL: convert_cnst_type not recognozed: '//convert_cnst_type
call endrun(sub//': FATAL: convert_cnst_type not recognozed: '//convert_cnst_type)
end if
if (.not.(convert_cnst_type == 'wet' .or. convert_cnst_type == 'dry')) then
write(iulog,*) sub//': FATAL: convert_cnst_type not recognized: '//convert_cnst_type
call endrun(sub//': FATAL: convert_cnst_type not recognized: '//convert_cnst_type)
end if

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for catching that. Fixed.

@cacraigucar cacraigucar changed the title fix CLUBB interface bug cam6_4_006: fix CLUBB interface bug Jul 2, 2024
@brian-eaton brian-eaton merged commit 9ca1ea4 into ESCOMP:cam_development Jul 3, 2024
gold2718 pushed a commit to gold2718/CAM that referenced this pull request Oct 16, 2024
Merge pull request ESCOMP#1054 from PeterHjortLauritzen/clubb_intr_bug_fix

cam6_4_006: fix CLUBB interface bug

ESCOMP commit: 9ca1ea4
gold2718 pushed a commit to gold2718/CAM that referenced this pull request Oct 16, 2024
Merge pull request ESCOMP#1054 from PeterHjortLauritzen/clubb_intr_bug_fix

cam6_4_006: fix CLUBB interface bug

ESCOMP commit: 9ca1ea4
gold2718 pushed a commit to gold2718/CAM that referenced this pull request Nov 8, 2024
Merge pull request ESCOMP#1054 from PeterHjortLauritzen/clubb_intr_bug_fix

cam6_4_006: fix CLUBB interface bug

ESCOMP commit: 9ca1ea4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answer changing answer changing tag bug-fix This PR was created to fix a specific bug. CoupledEval3
Projects
Status: Tag
Development

Successfully merging this pull request may close these issues.

6 participants