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

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

Merged
merged 19 commits into from
May 17, 2024

Conversation

grantfirl
Copy link
Collaborator

@grantfirl grantfirl commented May 3, 2024

This PR combines the following:

#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
#201 (credit @dustinswales)
#202 (credit @GeorgeGayno-NOAA)
#204 (credit @drnimbusrain)

Also, there are a couple of other bugfixes:

  • an updated CODEOWNERS file to update source file paths for automatically requesting reviews and to add a new person
  • a bug (openACC directive typo) was fixed in cu_gf_deep.F90 to fix failing NVidia compiler CI tests in the CCPP-SCM

DWesl and others added 10 commits April 9, 2024 17:35
The ozone photochemistry parametrization is a first-order Taylor expansion of the net ozone production in three variables.  This commit changes the four diagnostics the parametrization manages to match the four terms in the Taylor expansion.

When comparing to the implementation in lines 301-304, note that it is implicit in the model ozone mixing ratio, so line 301 has the first term and the second half of the second term, while the first half of the second term is part of the denominator on line 304.

I could use ozi or ozib for an explicit approximation to that term, but we already calculated the value needed for the implicit calculation actually used for the update.
…d some output variables in module_sf_noahmplsm subroutines to accomplish this)
@grantfirl
Copy link
Collaborator Author

@cenlinhe @barlage @drnimbusrain Please see 68ea163

@grantfirl grantfirl requested review from barlage and cenlinhe May 6, 2024 18:48
@cenlinhe
Copy link
Collaborator

cenlinhe commented May 6, 2024

@cenlinhe @barlage @drnimbusrain Please see 68ea163

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

@drnimbusrain
Copy link

@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
Copy link
Collaborator

barlage commented May 7, 2024

@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
Copy link
Collaborator Author

@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?

@grantfirl grantfirl marked this pull request as ready for review May 7, 2024 14:40
@drnimbusrain
Copy link

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
Copy link
Collaborator

barlage commented May 7, 2024

@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
Copy link
Collaborator

cenlinhe commented May 7, 2024

@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
Copy link
Collaborator

barlage commented May 7, 2024

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

@cenlinhe
Copy link
Collaborator

cenlinhe commented May 7, 2024

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

@grantfirl
Copy link
Collaborator Author

@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
Copy link
Collaborator

barlage commented May 7, 2024

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

@grantfirl
Copy link
Collaborator Author

grantfirl commented May 9, 2024

@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: grantfirl/noahmp@1e25901...12aef87

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
Copy link

@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
Copy link
Collaborator Author

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

@grantfirl
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

@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
Copy link

@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 grantfirl requested a review from mkavulich May 16, 2024 18:06
@grantfirl
Copy link
Collaborator Author

@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.

@grantfirl grantfirl requested a review from Qingfu-Liu May 16, 2024 18:26
@zach1221
Copy link

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

@dustinswales dustinswales merged commit 89ddce7 into ufs-community:ufs/dev May 17, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants