search

ufs-community / ccpp-physics

UFS fork for CCPP
Other
3 stars 32 forks source link

Combination PR for ozone diagnostics, metadata intent bugfixes, sfcsub.F landmask bugfix, and canopy resistance output #205

Closed grantfirl closed 2 months ago

grantfirl commented 2 months ago

This PR combines the following:

https://github.com/ufs-community/ccpp-physics/pull/196 (credit @DWesl) Note: changes baselines for tests that use diag_additional_control_dtend or diag_additional_rap_dtend diag tables: control_diag_debug_intel/gnu, rap_diag_debug_intel/gnu https://github.com/ufs-community/ccpp-physics/pull/201 (credit @dustinswales) https://github.com/ufs-community/ccpp-physics/pull/202 (credit @GeorgeGayno-NOAA) https://github.com/ufs-community/ccpp-physics/pull/204 (credit @drnimbusrain)

Also, there are a couple of other bugfixes:

grantfirl commented 2 months ago

@cenlinhe @barlage @drnimbusrain Please see https://github.com/ufs-community/ccpp-physics/pull/205/commits/68ea1635d07b025dcb247995ff16b5fbe94ca444

cenlinhe commented 2 months ago

@cenlinhe @barlage @drnimbusrain Please see 68ea163

The rca calculation in noahmp driver module looks good to me.

drnimbusrain commented 2 months ago

@cenlinhe @barlage @drnimbusrain Please see 68ea163

The rca calculation in noahmp driver module looks good to me.

Yes, seems OK to me too. Thank you!

barlage commented 2 months ago

@grantfirl @drnimbusrain @cenlinhe OK, this is fine with me for now to keep this moving. I probably would not have used the "effective" LAI. This is legacy from times when the LAI was scaled by vegetation fraction to prevent unrealistic values (this scaling has been removed). The limit of 6 is rather restrictive, but has no effect on the current model since the parameter table values max out at 4.5 so there will be no difference in the current model if LAISUN and LAISHA are used instead of effective values. Moving forward as we start using satellite inputs, which usually max out at 10, the "effective" LAI should be removed entirely.

grantfirl commented 2 months ago

@barlage @drnimbusrain @cenlinhe I would be happy to switch it to using the standard LAIs that are already being passed back. That minimizes the total changes, and we would only need the changes to pass back a valid value of the leaf boundary layer resistance. My changes were meant to replicate those from @drnimbusrain exactly. @drnimbusrain In your opinion, is there a reason to keep the "effective" values in the calculation of rca?

drnimbusrain commented 2 months ago

No reason to keep/use the "effective" values. I defer to Mike's suggestion, so its great if you can switch it to use the standard LAISUN and LAISHA. Thank you!

barlage commented 2 months ago

@grantfirl OK, if you can do this, I think it would be better since we will likely remove the "effective" part when we get the time varying LAI capability from satellite data working (@sanatcumar). I believe everything needed for the rca calculation should already be there since leaf_air_resistance is already coming out to noahmpdrv so we probably don't need any changes to module_sf_noahmplsm. This will also likely help with the component model noahmp syncing (@uturuncoglu)

cenlinhe commented 2 months ago

@barlage I thought the effective LAI and SAI also accounts for the snow-buried canopy effect. Would this be a concern if we use standard LAI and SAI?

barlage commented 2 months ago

laisun and laisha already use the other effective lai (elai)

cenlinhe commented 2 months ago

OK, I see. too many variables with similar names...

grantfirl commented 2 months ago

@grantfirl OK, if you can do this, I think it would be better since we will likely remove the "effective" part when we get the time varying LAI capability from satellite data working (@sanatcumar). I believe everything needed for the rca calculation should already be there since leaf_air_resistance is already coming out to noahmpdrv so we probably don't need any changes to module_sf_noahmplsm. This will also likely help with the component model noahmp syncing (@uturuncoglu)

@barlage Sure, I'll do this. Although it is true that leaf_air_resistance is coming into noahmpdrv_run, it is not actually being given a value other than 0. Following the logic in module_sf_noahmplsm, it goes as deep as noahmp_sflx()->energy() where it is initialized to 0, but it isn't given any other value. I see that it is calculated in vege_flux()->ragrb(), but it is local in vege_flux() and not returned to energy(). So, we'll need to maintain the code changes in module_sf_noahmplsm to at least make sure that leaf_air_resistance has a value in noahmpdrv_run.

barlage commented 2 months ago

@grantfirl ahh, OK, that looks like a wrf modification that didn't get incorporated in ccpp

grantfirl commented 2 months ago

@barlage @cenlinhe @drnimbusrain Finished. I'll test as part of the combined PR as soon as Hera is back up.

grantfirl commented 2 months ago

@barlage @cenlinhe @drnimbusrain Initial testing showed failures for the tests that call NOAHMP through the NUOPC interface. I suppose that this is expected since the interface for noahmpdrv_run was changed with the new variable. I started a new branch in the noahmp repo with changes to the NUOPC interface that should allow the model to at least run: https://github.com/grantfirl/noahmp/compare/1e259014c1eba9070cec7027d8b4b479ae54275a...12aef874d4b2064f3489e1637e6b182b0d70d8e8

Note that I didn't change lnd_comp_io.F90 because I don't know if there would be any further downstream effects.

I'm retesting again with the updates to the noahmp NUOPC interface.

drnimbusrain commented 2 months ago

@grantfirl Sorry, but I need to test this implementation of rca calculation. Seems to be giving strange values from my assessment. I will be in touch ASAP.

grantfirl commented 2 months ago

@drnimbusrain Thanks for letting me know. Please keep me in the loop if something needs to be changed.

grantfirl commented 2 months ago

@grantfirl Sorry, but I need to test this implementation of rca calculation. Seems to be giving strange values from my assessment. I will be in touch ASAP.

@drnimbusrain I think that part (all?) of the problem could be that rca is only calculated for non-glacier points. Since it is an intent(out) variable, it will be given a random value in memory for glacier points. If I initialize it to zero, perhaps this will fix what you're seeing? I'll do this and push real quick...

grantfirl commented 2 months ago

@grantfirl Sorry, but I need to test this implementation of rca calculation. Seems to be giving strange values from my assessment. I will be in touch ASAP.

@drnimbusrain I think that part (all?) of the problem could be that rca is only calculated for non-glacier points. Since it is an intent(out) variable, it will be given a random value in memory for glacier points. If I initialize it to zero, perhaps this will fix what you're seeing? I'll do this and push real quick...

@drnimbusrain Nevermind, the calculation of rca is outside of the if/then for glacier/non-glacier, so it should always be given a value.

drnimbusrain commented 2 months ago

@grantfirl Sorry, but I need to test this implementation of rca calculation. Seems to be giving strange values from my assessment. I will be in touch ASAP.

@drnimbusrain I think that part (all?) of the problem could be that rca is only calculated for non-glacier points. Since it is an intent(out) variable, it will be given a random value in memory for glacier points. If I initialize it to zero, perhaps this will fix what you're seeing? I'll do this and push real quick...

@drnimbusrain Nevermind, the calculation of rca is outside of the if/then for glacier/non-glacier, so it should always be given a value.

OK, thanks @grantfirl. I am still looking at the rca calculation itself and values. I will need to likely revise the conditions I have there. I'll be in touch soon.

grantfirl commented 2 months ago

@mkavulich @dustinswales @Qingfu-Liu Could one of you please review this combination PR when you get a chance. Note that there are a couple of changes since the approval by @Qingfu-Liu . One is an updated CODEOWNERS file to update source file paths for automatically requesting reviews and to add a new person. Another is that @drnimbusrain added limits to the calculation of the diagnostic canopy resistance. A third is that a bug (openACC directive typo) was fixed in cu_gf_deep.F90 to fix failing NVidia compiler CI tests in the CCPP-SCM.

zach1221 commented 2 months ago

@dustinswales regression testing is complete on WM PR#2264. Can you please merge this PR for us? Fyi, @grantfirl