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

Hook up drydep interface. #376

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

Hook up drydep interface. #376

wants to merge 17 commits into from

Conversation

overfelt
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Nov 17, 2024

Codecov Report

Attention: Patch coverage is 93.93939% with 4 lines in your changes missing coverage. Please review.

Project coverage is 95.87%. Comparing base (edaa167) to head (77f56d3).

Files with missing lines Patch % Lines
src/mam4xx/aero_model.hpp 83.33% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #376   +/-   ##
=======================================
  Coverage   95.86%   95.87%           
=======================================
  Files          45       45           
  Lines        9342     9352   +10     
=======================================
+ Hits         8956     8966   +10     
  Misses        386      386           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

const Real adv_mass_kg_per_moles[gas_pcnst],
const Real adv_mass_kg_per_moles[gas_pcnst], const Real mmr[gas_pcnst],
const Real fraction_landuse[mam4::mo_drydep::n_land_type],
const int col_index_season[mam4::mo_drydep::n_land_type],
Copy link
Contributor

@odiazib odiazib Nov 17, 2024

Choose a reason for hiding this comment

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

@overfelt For col_index_season, please see PR 3108. This PR was merged into the master branch in the scream repo. However, I do not see these changes in the master branch of E3SM. We may need to wait or request for these branches to be synced.

Copy link
Contributor

@mingxuanwupnnl mingxuanwupnnl left a comment

Choose a reason for hiding this comment

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

Thank you @overfelt. I just left some comments (may edit them later).

Comment on lines 186 to 191
for (int i = 0; i < NSeas; ++i) {
for (int j = 0; j < NLUse; ++j) {
data.rac(i, j) = rac_a[i][j];
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The way of initialization works on CPU but probably not for GPU. Need to fix it. @odiazib @singhbalwinder and I discussed it before, I @odiazib has ways to fix it.

Comment on lines 304 to 390
mam4::mo_drydep::drydep_xactive(
drydep_data,
fraction_landuse, // fraction of land use for column by land type
month, // month
col_index_season, // column-specific mapping of month indices to
// seasonal land-type indices [-]
sfc_temp, // surface temperature [K]
air_temp, // surface air temperature [K]
tv, // potential temperature [K]
pressure_sfc, // surface pressure [Pa]
pressure_10m, // 10-meter pressure [Pa]
spec_hum, // specific humidity [kg/kg]
wind_speed, // 10-meter wind spped [m/s]
rain, // rain content [??]
snow, // snow height [m]
solar_flux, // direct shortwave surface radiation [W/m^2]
mmr, // constituent MMRs [kg/kg]
dvel, // deposition velocity [1/cm/s]
dflx // deposition flux [1/cm^2/s]
);
Copy link
Contributor

@mingxuanwupnnl mingxuanwupnnl Nov 19, 2024

Choose a reason for hiding this comment

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

need to add if (kk == nlev) for drydep_xactive. Only gas dry dep fluxes and velocity are only calculated at bottom layer.

Some input vars need calculation. For example, wind_speed, it is calculated as sqrt(ubot^2 + vbot^2). Not sure which is prefered, I guess it would be better to calculations here in MAMxx code.

@singhbalwinder singhbalwinder force-pushed the jroverf/drydep_for_eamxx branch from 8652ecb to 29c29a5 Compare November 23, 2024 00:23
@singhbalwinder
Copy link
Contributor

I should have tested it before pushing. I will test it and fix the build errors.

}
Kokkos::fence();
for (int mm = 0; mm < gas_wetdep_cnt; mm++) {
int mm2 = wetdep_map[mm];
Copy link
Contributor

Choose a reason for hiding this comment

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

@overfelt @odiazib @mjs271 : I was getting an error (sethet: het_rates (wet dep) not set for het reaction number) [see line 498 of this file] and I added this fence. Please let me know if there is a better way to fix it. I believe the error is due to a race condition where abort becomes > 0.

Copy link
Contributor

@odiazib odiazib Nov 24, 2024

Choose a reason for hiding this comment

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

We should use team.team_barrier(), as this part of the code is within a parallel_for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since you are modifying the code anyway, doing a ThreadVectorRange inside a TeamThreadRange as on line 298 is the way to go. And, yes, team_barrier() is the right call in this case.

A team_barrier() right before the abort check on line 496 might be overkill, but there are already five team_barrier()s in this function, maybe one more would not hurt.

Comment on lines 361 to 380
mam4::mo_drydep::drydep_xactive(
drydep_data,
fraction_landuse, // fraction of land use for column by land type
month, // month
col_index_season, // column-specific mapping of month indices to
// seasonal land-type indices [-]
sfc_temp, // surface temperature [K]
air_temp, // surface air temperature [K]
tv, // potential temperature [K]
pressure_sfc, // surface pressure [Pa]
pressure_10m, // 10-meter pressure [Pa]
spec_hum, // specific humidity [kg/kg]
wind_speed, // 10-meter wind spped [m/s]
rain, // rain content [??]
snow, // snow height [m]
solar_flux, // direct shortwave surface radiation [W/m^2]
qq, // constituent MMRs [kg/kg]
dvel, // deposition velocity [1/cm/s]
dflx // deposition flux [1/cm^2/s]
);
Copy link
Contributor

Choose a reason for hiding this comment

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

gas dry dep is only executed for bottom layer. I think we need to add if condition here. Some of the input vars for dry dep needs calculation. Currently it is done in EAMxx branch. We might consider move them to MAMxx code if it is the preferred practice.

overfelt and others added 4 commits December 9, 2024 13:24
* Hook up drydep interface.

* Inequality needs to be >= to account for fortran -> C++ indexing.

* Only call mam4::mo_drydep::drydep_xactive on the bottom level: nlev-1

---------

Co-authored-by: Balwinder Singh <[email protected]>
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.

4 participants