Skip to content

Remove conversion and checkTemp helpers per TODO #3853

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

Open
Thomas-Sedhom wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Remove conversion and checkTemp helpers per TODO #3853

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5cb965c
Remove conversion and checkTemp helpers per TODO
Thomas-Sedhom Mar 6, 2026
e1bde55
Merge branch 'develop' into refactor/ed2-inline-conversion-checktemp
Thomas-Sedhom Mar 10, 2026
3826d22
Merge branch 'develop' into refactor/ed2-inline-conversion-checktemp
Thomas-Sedhom Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 13 additions & 26 deletions models/ed/R/model2netcdf.ED2.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,19 +752,6 @@ put_T_values <-
umol2kg_C <- Mc * PEcAn.utils::ud_convert(1, "umol", "mol") * PEcAn.utils::ud_convert(1, "g", "kg")
yr2s <- PEcAn.utils::ud_convert(1, "s", "yr")

# TODO - remove this function and replace with ifelse statements inline below (SPS)
Copy link
Copy Markdown
Member

@infotroph infotroph Mar 19, 2026

Choose a reason for hiding this comment

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

I didn't write this comment (that was probably @serbinsh years ago), but I recommend against it -- inline ifelse statements get messy fast and don't show intent as well as named helpers.

But while I'm looking at it, a higher-level fix that would eliminate the need for these operations would be to set all the missing values in out to literal NAs instead of -999. Then numeric conversions can be done on whole columns without worrying about disturbing the missing value flags, and the ncdf4 library will handle them automatically.

This suggestion does assume out is only written to netcdf and not passed to any other processes that need the missing values to be -999, so someone who knows the ED model should weigh in on whether that's true. Even if it does get passed elsewhere, it may still be cleaner overall to convert the NAs once at the end rather than work around them when converting each column.

Copy link
Copy Markdown
Contributor Author

@Thomas-Sedhom Thomas-Sedhom Mar 22, 2026

Choose a reason for hiding this comment

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

I took a closer look at the data flow in model2netcdf.ED2.R. From what I found, the out object built by read_T_files() / read_E_files() is mainly used as an intermediate step before writing to NetCDF, and I did not find any internal code path in this repo where it is passed to other ED processing that specifically depends on -999.
I also noticed that the E-file path already seems to rely on NA internally in some places and still writes NetCDF variables with missval = -999, so your suggestion looks reasonable to me. The main remaining concern seems to be whether changing read_T_files() output from -999 to NA would affect any external callers, since these helper functions are exported. If that concern is not important, I agree that using NA internally and only handling missing values at the NetCDF boundary would likely be cleaner.

conversion <- function(col, mult) {
## make sure only to convert those values that are not -999
out[[col]][out[[col]] != -999] <- out[[col]][out[[col]] != -999] * mult
return(out)
}

checkTemp <- function(col) {
out[[col]][out[[col]] == 0] <- -999
return(out)
}


# ----- define ncdf dimensions
#### setup output time and time bounds
## Create a date vector that contains each day of the model run for each output year (e.g. "2001-07-15", "2001-07-16"....)
Expand Down Expand Up @@ -815,10 +802,10 @@ put_T_values <-

# ----- fill list

out <- conversion(1, PEcAn.utils::ud_convert(1, "t ha-1", "kg m-2")) ## tC/ha -> kg/m2
out[[1]] <- ifelse(out[[1]] != -999, out[[1]] * PEcAn.utils::ud_convert(1, "t ha-1", "kg m-2"), out[[1]]) ## tC/ha -> kg/m2
nc_var[[s + 1]] <- ncdf4::ncvar_def("AbvGrndWood", units = "kg C m-2", dim = list(lon, lat, t), missval = -999,
longname = "Above ground woody biomass")
out <- conversion(2, umol2kg_C) ## umol/m2 s-1 -> kg/m2 s-1
out[[2]] <- ifelse(out[[2]] != -999, out[[2]] * umol2kg_C, out[[2]]) ## umol/m2 s-1 -> kg/m2 s-1
nc_var[[s + 2]] <- ncdf4::ncvar_def("AutoResp", units = "kg C m-2 s-1", dim = list(lon, lat, t), missval = -999,
longname = "Autotrophic Respiration")
nc_var[[s + 3]] <- ncdf4::ncvar_def("CarbPools", units = "kg C m-2", dim = list(lon, lat, t), missval = -999,
Expand All @@ -827,19 +814,19 @@ put_T_values <-
longname = "CO2CAS")
nc_var[[s + 5]] <- ncdf4::ncvar_def("CropYield", units = "kg m-2", dim = list(lon, lat, t), missval = -999,
longname = "Crop Yield")
out <- conversion(6, yr2s) ## kg C m-2 yr-1 -> kg C m-2 s-1
out[[6]] <- ifelse(out[[6]] != -999, out[[6]] * yr2s, out[[6]]) ## kg C m-2 yr-1 -> kg C m-2 s-1
nc_var[[s + 6]] <- ncdf4::ncvar_def("GPP", units = "kg C m-2 s-1", dim = list(lon, lat, t), missval = -999,
longname = "Gross Primary Productivity")
out <- conversion(7, yr2s) ## kg C m-2 yr-1 -> kg C m-2 s-1
out[[7]] <- ifelse(out[[7]] != -999, out[[7]] * yr2s, out[[7]]) ## kg C m-2 yr-1 -> kg C m-2 s-1
nc_var[[s + 7]] <- ncdf4::ncvar_def("HeteroResp", units = "kg C m-2 s-1", dim = list(lon, lat, t), missval = -999,
longname = "Heterotrophic Respiration")
out <- conversion(8, yr2s) ## kg C m-2 yr-1 -> kg C m-2 s-1
out[[8]] <- ifelse(out[[8]] != -999, out[[8]] * yr2s, out[[8]]) ## kg C m-2 yr-1 -> kg C m-2 s-1
nc_var[[s + 8]] <- ncdf4::ncvar_def("NEE", units = "kg C m-2 s-1", dim = list(lon, lat, t), missval = -999,
longname = "Net Ecosystem Exchange")
out <- conversion(9, yr2s) ## kg C m-2 yr-1 -> kg C m-2 s-1
out[[9]] <- ifelse(out[[9]] != -999, out[[9]] * yr2s, out[[9]]) ## kg C m-2 yr-1 -> kg C m-2 s-1
nc_var[[s + 9]] <- ncdf4::ncvar_def("NPP", units = "kg C m-2 s-1", dim = list(lon, lat, t), missval = -999,
longname = "Net Primary Productivity")
out <- conversion(10, yr2s) ## kg C m-2 yr-1 -> kg C m-2 s-1
out[[10]] <- ifelse(out[[10]] != -999, out[[10]] * yr2s, out[[10]]) ## kg C m-2 yr-1 -> kg C m-2 s-1
nc_var[[s + 10]] <- ncdf4::ncvar_def("TotalResp", units = "kg C m-2 s-1", dim = list(lon, lat, t), missval = -999,
longname = "Total Respiration")
nc_var[[s + 11]] <- ncdf4::ncvar_def("TotLivBiom", units = "kg C m-2", dim = list(lon, lat, t), missval = -999,
Expand All @@ -865,7 +852,7 @@ put_T_values <-
longname = "Rainfall rate")
nc_var[[s + 22]] <- ncdf4::ncvar_def("SWdown", units = "W m-2", dim = list(lon, lat, t), missval = -999,
longname = "Surface incident shortwave radiation")
out <- checkTemp(23)
out[[23]] <- ifelse(out[[23]] == 0, -999, out[[23]])
nc_var[[s + 23]] <- ncdf4::ncvar_def("Tair", units = "K", dim = list(lon, lat, t), missval = -999,
longname = "Near surface air temperature")
nc_var[[s + 24]] <- ncdf4::ncvar_def("Wind", units = "m s-1", dim = list(lon, lat, t), missval = -999,
Expand All @@ -876,7 +863,7 @@ put_T_values <-
longname = "Ground heat")
nc_var[[s + 27]] <- ncdf4::ncvar_def("Qh", units = "W m-2", dim = list(lon, lat, t), missval = -999,
longname = "Sensible heat")
out <- conversion(28, PEcAn.data.atmosphere::get.lv()) ## kg m-2 s-1 -> W m-2
out[[28]] <- ifelse(out[[28]] != -999, out[[28]] * PEcAn.data.atmosphere::get.lv(), out[[28]]) ## kg m-2 s-1 -> W m-2
nc_var[[s + 28]] <- ncdf4::ncvar_def("Qle", units = "W m-2", dim = list(lon, lat, t), missval = -999,
longname = "Latent heat")
nc_var[[s + 29]] <- ncdf4::ncvar_def("SWnet", units = "W m-2", dim = list(lon, lat, t), missval = -999,
Expand All @@ -894,25 +881,25 @@ put_T_values <-
nc_var[[s + 36]] <- PEcAn.utils::mstmipvar("SMLiqFrac", lat, lon, t, zg) # not standard
nc_var[[s + 37]] <- ncdf4::ncvar_def("SoilMoist", units = "kg m-2", dim = list(lon, lat, zg, t), missval = -999,
longname = "Average Layer Soil Moisture")
out <- checkTemp(38)
out[[38]] <- ifelse(out[[38]] == 0, -999, out[[38]])
nc_var[[s + 38]] <- ncdf4::ncvar_def("SoilTemp", units = "K", dim = list(lon, lat, zg, t), missval = -999,
longname = "Average Layer Soil Temperature")
nc_var[[s + 39]] <- ncdf4::ncvar_def("SoilWet", units = "", dim = list(lon, lat, t), missval = -999,
longname = "Total Soil Wetness")
nc_var[[s + 40]] <- PEcAn.utils::mstmipvar("Albedo", lat, lon, t, zg) # not standard
out <- checkTemp(41)
out[[41]] <- ifelse(out[[41]] == 0, -999, out[[41]])
nc_var[[s + 41]] <- PEcAn.utils::mstmipvar("SnowT", lat, lon, t, zg) # not standard
nc_var[[s + 42]] <- ncdf4::ncvar_def("SWE", units = "kg m-2", dim = list(lon, lat, t), missval = -999,
longname = "Snow Water Equivalent")
out <- checkTemp(43)
out[[43]] <- ifelse(out[[43]] == 0, -999, out[[43]])
nc_var[[s + 43]] <- PEcAn.utils::mstmipvar("VegT", lat, lon, t, zg) # not standard
nc_var[[s + 44]] <- ncdf4::ncvar_def("Evap", units = "kg m-2 s-1", dim = list(lon, lat, t), missval = -999,
longname = "Total Evaporation")
nc_var[[s + 45]] <- ncdf4::ncvar_def("Qs", units = "kg m-2 s-1", dim = list(lon, lat, t), missval = -999,
longname = "Surface runoff")
nc_var[[s + 46]] <- ncdf4::ncvar_def("Qsb", units = "kg m-2 s-1", dim = list(lon, lat, t), missval = -999,
longname = "Subsurface runoff")
out <- conversion(47, yr2s) ## kg C m-2 yr-1 -> kg C m-2 s-1
out[[47]] <- ifelse(out[[47]] != -999, out[[47]] * yr2s, out[[47]]) ## kg C m-2 yr-1 -> kg C m-2 s-1
nc_var[[s + 47]] <- ncdf4::ncvar_def("SoilResp", units = "kg C m-2 s-1", dim = list(lon, lat, t), missval = -999,
longname = "Soil Respiration")
# Remove SLZ from output before finalizing list. replace with time_bounds
Expand Down
Loading