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.

Sign up for GitHub

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

Jump to bottom

User/f1p/merge gex ddep #151

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

User/f1p/merge gex ddep #151

wants to merge 24 commits into from

Conversation

fabienpaulot
Copy link
Contributor

Modifications to:
a) handle dry deposition on land side
b) fix error in ocean flux diagnostics for vmr tracers (NH3 only in ESM4p5)
c) pass fluxes (not prognostic tracers) between land and atmosphere (gex)

Depends on esm4.5_ddep_gex tag (FMS, atmos_drivers)

slm7826 and others added 13 commits March 30, 2021 21:02
@slm7826
Pass con_atm to land, for dry deposition calculations
d385cbe
@fabienpaulot
add diagnostics for tracer concentration at ref height
cb3f5ab
@slm7826
Fix OMP shared var spec
ca6139f
@slm7826
Changes from Fabien
987e6e9 
See his e-mail of July 15, 2022
@slm7826
Merge tag '2022.04' into user/f1p/sfcx_ddep
8558289 
Tag for 2022.04 release
@slm7826
Rename get_from_xgrid_land to match current conventions
fb3fc4f 
Apparently the subroutine called "get_from_xgrid_land" in older
versions of coupler/xgrid was renamed "get_from_xgrid_ug" in
some update. In this commit few references to the old name that
were left in by merge are fixed to use the new name.
@slm7826
Merge tag '2024.01' into HEAD
cfd2e61 
Tag for 2024.01 version 1 of FMScoupler
@fabienpaulot
changes for gex
590f99c
@fabienpaulot
bug fix deposition
c323f7b
@fabienpaulot
Merge branch 'user/f1p/bug_fix' into user/f1p/gex
dfb2d83
@fabienpaulot
use n_gex instead of n to avoid collisions with other fields. Not sur…
6c25d46 
…e why this would be needed
@fabienpaulot
fix error n->n_gex
2d6f41b
@fabienpaulot
changes for dry deposition. fix error in ocean flux for vmr tracers
1aa99f1
@rem1776
Copy link
Contributor

rem1776 commented Jan 27, 2025 •
edited
Loading

Just want to note that this will need NOAA-GFDL/FMS#1637 merged in first before the CI will pass, since this depends on the added module.

@fabienpaulot This will need some line length and trailing whitespace clean up to pass the linter before we can merge it. The linter output is here but you can also find it in the checks tab, it'll tell you which files need to have overly long lines or trailing whitespace fixed.

@fabienpaulot
Copy link
Contributor Author

fabienpaulot commented Jan 27, 2025 via email

@rem1776
Copy link
Contributor

rem1776 commented Jan 27, 2025

@fabienpaulot The linter action only lets us know, but you can use grep to get an exact line number:

grep -n " $" <filenames>

for trailing whitespace.

grep -n '.\{120\}' <filenames>

for line lengths

@fabienpaulot
Copy link
Contributor Author

fabienpaulot commented Jan 27, 2025 via email

@fabienpaulot
Copy link
Contributor Author

This will need NOAA-GFDL/atmos_drivers#49 (comment) before this can be merged

thomas-robinson
thomas-robinson requested changes Jan 28, 2025
View reviewed changes
Copy link
Member

@thomas-robinson thomas-robinson 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 that all of the variables you add are documented using doxygen. I realize that there are hundreds of undocumented variables, but we request new variables are documented appropriately

full/atm_land_ice_flux_exchange.F90 Outdated
PI, CP_OCEAN, WTMCO2, WTMC, EPSLN, GRAV
PI, CP_OCEAN, WTMCO2, WTMC, EPSLN, GRAV, WTMH2O

!gex
Copy link
Member

Choose a reason for hiding this comment

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

The comment should be removed or expanded. It doesn't provide any useful information.

full/atm_land_ice_flux_exchange.F90 Outdated
Comment on lines 250 to 255
!map atm tracers to exchange, ice and land variables
type(tracer_exch_ind_type), allocatable :: tr_table_map(:)
!index of tracers in the array of tracers passed through flux exchange (tr_table)
integer :: isphum = NO_TRACER !< specific humidity
integer :: ico2 = NO_TRACER !< co2 tracer
integer :: inh3 = NO_TRACER !< nh3 tracer
Copy link
Member

Choose a reason for hiding this comment

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

Please make sure all documentation is in doxygen format.

full/atm_land_ice_flux_exchange.F90 Outdated
@@ -302,6 +318,7 @@ subroutine atm_land_ice_flux_exchange_init(Time, Atm, Land, Ice, atmos_ice_bound
real, intent(in) :: z_ref_heat_in, z_ref_mom_in
logical, intent(in) :: scale_precip_2d_in
logical, intent(in) :: do_area_weighted_flux_in
logical, intent(in) :: ocn_atm_flux_vmr_bug_in
Copy link
Member

Choose a reason for hiding this comment

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

Please document in doxygen format

full/flux_exchange.F90 Outdated
@@ -566,10 +566,12 @@ module flux_exchange_mod
!! tfreeze parameter
real, parameter :: tfreeze = 273.15
logical :: scale_precip_2d = .false.
logical :: ocn_atm_flux_vmr_bug = .true. !set to .true. to reproduce old (erroneous) conversion
Copy link
Member

Choose a reason for hiding this comment

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

Please use doxygen format

full/atm_land_ice_flux_exchange.F90 Outdated
Comment on lines 726 to 727
real :: rho
real, dimension(n_xgrid_sfc,n_exch_tr) :: ex_tr_atm, ex_tr_ref
Copy link
Member

Choose a reason for hiding this comment

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

Please document these new variables in doxygen format

full/atm_land_ice_flux_exchange.F90 Outdated
Comment on lines 223 to 224
!deposition velocity at reference height and atmospheric height
real, allocatable, dimension(:,:) :: ex_tr_con_ref, ex_tr_con_atm
Copy link
Member

Choose a reason for hiding this comment

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

Please switch the documentation to doxygen format. The variables below show an example of the format using !< for each variable.

full/atm_land_ice_flux_exchange.F90 Outdated
real, parameter :: tfreeze = 273.15
real, allocatable, dimension(:,:) :: frac_precip

!--- the following is from flux_exchange_nml
real :: z_ref_heat = 2. !< Reference height (meters) for temperature and relative humidity diagnostics
!! (t_ref, rh_ref, del_h, del_q)
real :: z_ref_mom = 10. !< Reference height (meters) for mementum diagnostics (u_ref, v_ref, del_m)
logical :: ocn_atm_flux_vmr_bug
Copy link
Member

Choose a reason for hiding this comment

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

Please document this variable using a doxygen-format comment

full/atm_land_ice_flux_exchange.F90 Outdated
Comment on lines 129 to 130
integer, allocatable :: id_tr_con_atm_land(:), id_tr_con_ref_land(:)
integer, allocatable :: id_tr_con_atm(:), id_tr_con_ref(:)
Copy link
Member

Choose a reason for hiding this comment

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

Please add doxygen-style documentation for new variables

@fabienpaulot
Copy link
Contributor Author

fabienpaulot commented Jan 28, 2025 via email •
edited by thomas-robinson
Loading

@thomas-robinson
Copy link
Member

@fabienpaulot it looks like there are some issues with line length still

@fabienpaulot
Copy link
Contributor Author

Thanks @thomas-robinson for bearing with me. It looks like I finally passed the linter check.

For my reference, here are the two functions I needed to use:

grep '[[:space:]]$' -n FILENAME
grep -n ".{121}" FILENAME [I was using 120 before]

uramirez8707
uramirez8707 requested changes Mar 10, 2025
View reviewed changes
full/atm_land_ice_flux_exchange.F90 Outdated
Comment on lines 2527 to 2531
if (associated(Land_boundary%gex_fields)) then
do n_gex=1,n_gex_atm2lnd
call fms_xgrid_get_from_xgrid (Land_boundary%gex_fields(:,:,n_gex), 'LND', ex_gex_atm2lnd(:,n_gex), xmap_sfc)
end do
end if
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be Land_boundary%gex_atm2lnd so that it can be consistent with L2493-L2502 above?

@fabienpaulot
Copy link
Contributor Author

fabienpaulot commented Mar 10, 2025 via email

uramirez8707
uramirez8707 reviewed Mar 10, 2025
View reviewed changes
full/atm_land_ice_flux_exchange.F90
Comment on lines +1872 to +1879
if(id_tr_ref_land(tr) > 0) then
call fms_xgrid_get_from_xgrid_ug (diag_land, 'LND', ex_tr_ref(:,tr), xmap_sfc)
!duplicate send_tile_data. We may remove id_q_ref_land in the future.
#ifndef _USE_LEGACY_LAND_
call send_tile_data (id_tr_ref_land(tr), diag_land)
#else
used = fms_diag_send_tile_averaged_data(id_tr_ref_land(tr), diag_land, &
Land%tile_size, Time, mask=Land%mask)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to compile:

/__w/FMScoupler/FMScoupler/t/null.Iu6wiF/src/coupler/full/atm_land_ice_flux_exchange.F90:1873:88:

 1873 |           call fms_xgrid_get_from_xgrid_ug (diag_land, 'LND', ex_tr_ref(:,tr), xmap_sfc)
      |                                                                                        1
Error: There is no specific subroutine for the generic 'fms_xgrid_get_from_xgrid_ug' at (1)

@fabienpaulot
Copy link
Contributor Author

fabienpaulot commented Mar 10, 2025 via email

@rem1776
Copy link
Contributor

rem1776 commented Mar 12, 2025

@fabienpaulot Let me know if your still running into issues merging, me or someone else from our team should be able to help out. There was a commit added here that might be missing on your local branch, so you might need to pull in the remote branch (git pull origin user/f1p/merge_gex_ddep) before committing.

@fabienpaulot
Copy link
Contributor Author

fabienpaulot commented Mar 12, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uramirez8707 uramirez8707 uramirez8707 requested changes

@thomas-robinson thomas-robinson thomas-robinson requested changes

@bensonr bensonr Awaiting requested review from bensonr

@rem1776 rem1776 Awaiting requested review from rem1776

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fabienpaulot @rem1776 @thomas-robinson @uramirez8707 @slm7826