USEPA / spsurvey

spsurvey: Spatial Sampling Design and Analysis in R
https://usepa.github.io/spsurvey/
GNU General Public License v3.0
15 stars 5 forks source link

Uncaught error in relrisk_analysis from unnamed stressor_levels and response_levels lists #33

Closed bodacious-bill closed 1 year ago

bodacious-bill commented 1 year ago

I just helped a colleague troubleshoot this, from some code that needed updating from a very old version of spsurvey to the current.

Their inputs were formatted as such: response_levels <- rep(list(c("I", "NI")), length(vars_response))

but the list was unnamed. Calling name(response_levels) would return NULL.

This was missed by all of the error checking on response_levels and stressor_levels. Some of the error checks wouldn't work as intended, since calling names() returns NULL.

Adding proper names to his list based on the list of vars_response or vars_stressor fixed the issue

I propose another couple of simple error checks, to check if names exist on these input lists, and either assigning names automatically, or throwing an error asking for names.

I can work on this PR (my first!) myself if needed. Also, I'm not sure what (if any) other functions would similarly benefit from this checking.

michaeldumelle commented 1 year ago

Thanks for the suggestion @bodacious-bill, and sorry for the late response (I am out on parental leave). I will incorporate this suggestion in the next version of spsurvey submitted to CRAN (the current version of spsurvey is v5.4.1). In the next version, if names(response_levels) returns NULL, the names of response_levels will be set to vars_response. This change implies that if there are no names given for response_levels, it is assumed that the elements in response_levels correspond, in order, to the variables specified by vars_response. Similarly, if names(stressor_levels) returns NULL, the names of stressor_levels will be set to vars_stressor. These changes will be reflected in attrisk_analysis(), diffrisk_analysis(), and relrisk_analysis().

michaeldumelle commented 1 year ago

@bodacious-bill this suggestion has been pushed to CRAN as part of v5.5.0.