NIEHS / amadeus

https://niehs.github.io/amadeus/
Other
7 stars 1 forks source link

Wrapper functions: one-shot calc_covariates or proc_covariates & calc_covariates? #14

Closed sigmafelix closed 9 months ago

sigmafelix commented 9 months ago

Old calculate_covariates used path, sites, id_col as common arguments. This approach made sense as previous calc_* functions were a combination of processing (or importing) and calculating functions. However, the wrapper will not work as we split these into two parts, which makes me think about calc(ulate)_covariates wrapper function refactoring ideas.

To deal with data-specific arguments in a wrapper function, I found that the combined use of ellipsis argument (...) and rlang::inject(foo(!!!args)) is helpful for development, which is reflected in my 0.1.0 PR #13 .

mitchellmanware commented 9 months ago

My original thoughts were the second option, where the workflow for all data sources is download_data() ... process_data() ... calculate_covariates(). Although the intermediate processing step may simply be reading in the data with terra::rast(), I think the consistency is helpful (at least for me).

Why would process_data() and calculate_covariates() wrapper functions not work the same as in data_download()? The data download wrapper function is able to accept a range of arguments based on the underlying specific function, so how would the others be different?

sigmafelix commented 9 months ago

@mitchellmanware Wrapper functions for process_* make sense. I look forward to finding that the ongoing PR is finalized then I will work on adding a wrapper.

sigmafelix commented 9 months ago

process_covariates is in progress along with adding process_sedac_groads. Will make a PR as soon as I finish adding these.

sigmafelix commented 9 months ago

Resolved by #26 .