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

ZM cleanup - Remove buoyan() subroutine #6964

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

whannah1
Copy link
Contributor

@whannah1 whannah1 commented Feb 1, 2025

This is a continuation of the ZM overhaul and refactoring, which is limited to cosmetic cleanup of the main ZM routines. In this iteration we remove the unused subroutine buoyan(), which is the original undilute parcel and CAPE calculations that were abandoned in the CAM4 era.

[BFB]

@whannah1 whannah1 added Atmosphere BFB PR leaves answers BFB ZM labels Feb 1, 2025
@whannah1 whannah1 requested a review from crterai February 1, 2025 00:30
@whannah1 whannah1 changed the title remove buoyan() subroutine from ZM Remove buoyan() subroutine from ZM Feb 1, 2025
@whannah1 whannah1 marked this pull request as draft February 1, 2025 00:30
@whannah1 whannah1 changed the title Remove buoyan() subroutine from ZM ZM cleanup - Remove buoyan() subroutine Feb 1, 2025
tp ,qstp ,tl ,rl ,cape , &
pblt ,lcl ,lel ,lon ,maxi , &
rgas ,grav ,cpres ,msg , &
tpert ,iclosure )
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it useful to keep info about intents (in, out, inouts) from a readability perspective. It allows us to find out which quantities are being modified and reason about whether they should be modified or not. If we want to avoid verbosity, we can collect all the intent-ins at the beginning, then inouts, and then intent-outs. This way, we will be adding comments in only three places:

call sub( !ins
....
!inouts
....
!outs
....)

The majority of the calls may have just ins and outs. You can add this info any way you like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't agree on this sentiment. While I do agree that we need information about which variables are ins/outs, in my view the "correct" place to put this is in the interface of the subroutine and not outlined at every call of a subroutine. The calls for these routines generally only appear once, but for other subroutines we don't label the intent of each variable at the place it is called, so I don't see why we would want to make an exception here. If someone is modifying one of these routines it will be their responsibility to understand the intent of the arguments as defined in the subroutine and modify all calls accordingly. Labelling the calls with intent value is just one of those things where future developers will be lazy and not update the comments to match the actual intent defined in the subroutine interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I see both perspectives here. I agree that it needs to be maintained (we were enforcing this during the review process in the MAM4 refactor). It may just be a personal preference, but I like to see the "intents" while reading the code to quickly assess whether it makes sense for this call to change these variables or not (without the need to search for the subroutine interface, which might be in another file). It also helps while debugging to immediately know if the variable we are tracing is being modified by a subroutine call or not. That said, I will leave it up to you, I am okay either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get that it's somewhat of a style issue, and not really objectively superior either way, but if it's up to me I'll leave them out on the call lines.

@whannah1
Copy link
Contributor Author

whannah1 commented Feb 3, 2025

The following tests pass on Chrysalis:

  e3sm_atm_developer_intel
  e3sm_atm_developer_gnu
  SMS_Ld32.ne30pg2_r05_oECv3.F2010.chrysalis_intel
  SMS_Ld32.ne30pg2_r05_oECv3.F2010.chrysalis_gnu
  SMS_Ld32.ne4pg2_oQU480.F2010.chrysalis_intel
  SMS_Ld32.ne4pg2_oQU480.F2010.chrysalis_gnu

The performance of the SMS_Ld32 tests are indentical, here's on of the ZM timers from SMS_Ld32.ne30pg2_r05_oECv3.F2010.chrysalis_intel:

              name                                            on  processes  threads        count      walltotal   wallmax (proc   thrd  )   wallmin (proc   thrd  )
    base: "a:zm_conv_tend"                                 -        128      256 2.360832e+06   1.579500e+04    72.009 (     1      0)    51.040 (    27      0)
    test: "a:zm_conv_tend"                                 -        128      256 2.360832e+06   1.579500e+04    72.009 (     1      0)    51.040 (    27      0)

@whannah1 whannah1 marked this pull request as ready for review February 3, 2025 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Atmosphere BFB PR leaves answers BFB ZM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants