remove base/utils/data/trait.dictionary.csv

infotroph commented 6 years ago

Description

Context

As discussed in #1713

Possible Implementation

To remove trait.dictionary.csv, probably need to edit the following files (as determined by $(grep -R 'trait.dictionary' pecan/):

[x] base/db/R/get.trait.data.R: Remove option to use trait.dictionary
[ ] base/utils/R/utils.R: Delete trait.lookup function entirely? Make it perform a bety query?
[ ] base/utils/tests/testthat/test.trait.dictionary.R: Remove or rewrite to match updated function
[ ] book_source/developers_guide/Package-data.Rmd, modules/assim.batch/vignettes/AssimBatchVignette.Rmd: Update documentation to match current practice

infotroph commented 6 years ago

See also #729

ashiklom commented 5 years ago

Bump! I would like to see trait.dictionary go as well.

Here is the block of get.trait.data that uses the trait dictionary. In the absence of trait.dictionary, what do we think is a sensible default? My vote would be to use all traits that have a prior for the given PFT (based on a BETY query), but I am open to other ideas as well.

Here's a prototype of what a function that retrieves the priors for a given PFT might look like (from my current ED2 work).

Pinging @robkooper @dlebauer @serbinsh as the function authors and @mdietze as benevolent overlord.

github-actions[bot] commented 4 years ago

This issue is stale because it has been open 365 days with no activity.

meetagrawal09 commented 1 year ago

To fix this one (according to points by @infotroph) :

Should we consider modifying trait.lookup function (by making it dependent on DB) or completely remove it ?
Should the test for trait.dictionary go away since we want to completely get rid of trait.dictionary ?

base/db/R/get.trait.data.R: Remove option to use trait.dictionary

I suppose this has been fixed. If someone can please confirm on that (and point to the right direction).

mdietze commented 1 year ago

AFAIK this has not been addressed, as there are clearly many uses of trait.lookup still in the code

grep -ir "trait.lookup" .
Binary file ./.git/index matches
./modules/priors/R/plots.R:    scale_x_continuous(limits = xlim, breaks = x.breaks, name = PEcAn.utils::trait.lookup(trait)$units) +
./modules/priors/R/plots.R:    labs(title = PEcAn.utils::trait.lookup(trait)$figid) +
./modules/priors/R/plots.R:  units <- PEcAn.utils::trait.lookup(trait)$units
./modules/priors/R/plots.R:    scale_x_continuous(limits = range(x.ticks), breaks = x.ticks, name = PEcAn.utils::trait.lookup(trait)$units) +
./modules/priors/R/plots.R:    labs(title = PEcAn.utils::trait.lookup(trait)$figid) +
./modules/assim.batch/R/plot.da.R:    trait.entry <- PEcAn.utils::trait.lookup(gsub("[1-2]$", "", traits[i]))
./modules/assim.batch/R/plot.da.R:      trait.entry <- PEcAn.utils::trait.lookup(traits[i])
./modules/assim.batch/R/plot.da.R:    trait.entry <- PEcAn.utils::trait.lookup(gsub("[1-2]$", "", traits[i]))
./modules/assim.batch/R/plot.da.R:      trait.entry <- PEcAn.utils::trait.lookup(traits[i])
./modules/uncertainty/tests/Rcheck_reference.log:  ‘trait.lookup’
./modules/uncertainty/tests/Rcheck_reference.log:  ‘trait.lookup’
./modules/uncertainty/tests/Rcheck_reference.log:  ‘trait.lookup’
./modules/uncertainty/tests/Rcheck_reference.log:  ‘trait.lookup’
./modules/uncertainty/tests/Rcheck_reference.log:  theme_set trait.lookup trait.samples trait.mcmc var variances vecpaste
./modules/uncertainty/R/plots.R:  units <- as.character(PEcAn.utils::trait.lookup(traits)$units)
./modules/uncertainty/R/plots.R:  trait.labels <- as.character(PEcAn.utils::trait.lookup(traits)$figid)
./modules/uncertainty/R/plots.R:                          trait.labels = PEcAn.utils::trait.lookup(traits)$figid, 
./modules/uncertainty/R/plots.R:                          units = PEcAn.utils::trait.lookup(traits)$units, 
./modules/uncertainty/R/plots.R:  units <- PEcAn.utils::trait.lookup(trait)$units
./modules/uncertainty/R/run.sensitivity.analysis.R:          C.units <- grepl('^Celsius$', trait.lookup(traits)$units, ignore.case = TRUE)
./git.log:    replaced use of trait.dictionary function with trait.lookup refs #1258
./git.log:    renamed function trait.dictionary to trait.lookup, in order to avoid problems related to the use of the same name for the function as for the dictionary
./.Rproj.user/D2C5EDB0/console06/C7321C64: base/utils/man/trait.lookup.Rd                                         |     4 +-
./base/utils/man/trait.lookup.Rd:\name{trait.lookup}
./base/utils/man/trait.lookup.Rd:\alias{trait.lookup}
./base/utils/man/trait.lookup.Rd:trait.lookup(traits = NULL)
./base/utils/man/trait.lookup.Rd:trait.lookup('growth_resp_factor')
./base/utils/man/trait.lookup.Rd:trait.lookup('growth_resp_factor')$figid
./base/utils/man/trait.lookup.Rd:trait.lookup()[,c('figid', 'units')]
./base/utils/R/utils.R:##' trait.lookup('growth_resp_factor')
./base/utils/R/utils.R:##' trait.lookup('growth_resp_factor')$figid
./base/utils/R/utils.R:##' trait.lookup()[,c('figid', 'units')]
./base/utils/R/utils.R:trait.lookup <- function(traits = NULL) {
./base/utils/R/utils.R:} # trait.lookup
./base/utils/NAMESPACE:export(trait.lookup)

That said, I think the general agreement in recent years has been to try and reduce the extent to which pecan depends on the database, not increase it. I'm not sure what the best course of action is in this particular case.

infotroph commented 1 year ago

I'll re-raise Mike's 2017 suggestion that many of the existing uses could be removed by using the variable names as-is rather than trying to map them to prettier strings.

A more ambitious approach would be to notice that the netCDF format encourages every variable to have a name, a long_name, and defined units; perhaps the PEcAn data standard should enforce that every model populates these in the output files and then our downstream code can look them up from there instead of the database.

dlebauer commented 1 year ago

Yep it looks like, aside from the trait.lookup function itself all of the uses are for plotting

PecanProject / pecan