bcgov / wqbc

An R package for water quality thresholds and index calculation for British Columbia
http://bcgov.github.io/wqbc/
Apache License 2.0
19 stars 9 forks source link

`clean_wqdata()` is incorrectly changing detected values to non-detects #164

Open KarHarker opened 2 years ago

KarHarker commented 2 years ago

This was previously reported but the error was not fixed by the standardize_mdl_units() rems function https://github.com/bcgov/wqbc/issues/158

I think this is partly happening because the columns change names in standardize_wqdata() and are not recognized in the rems function.

I think the best way to fix might be to:

...but open to other thoughts!

Also an issue identified in shinyrems https://github.com/bcgov/shinyrems/issues/123

aylapear commented 1 year ago

Hey @KarHarker, I looked into the issue and agree that we should pull the standardize_mdl_units() function into standardize_wqdata(). Since one of the purposes of standardize_wqdata() is to deal with units, it makes the most sense to put standardize_mdl_units() in the standardize function.

I wanted to confirm that this is the general workflow:

# pull ems data
two_year <- rems::get_ems_data(which = "2yr", ask = FALSE)
# filter ems data
data <- rems::filter_ems_data(two_year, emsid = c("0121580"))
# tidy data
data <- wqbc::tidy_ems_data(data)
# standardize data
data <- wqbc::standardize_wqdata(data)
# clean data
data <- wqbc::clean_wqdata(data, by = "EMS_ID")
# calc limit
data <- wqbc::calc_limits(data, by = "EMS_ID", term = "short")

If this is the workflow, then we will need to ensure the wqbc::tidy_ems_data() function keeps the MDL_UNIT column. We can keep the MDL_UNIT column in the output of wqbc::standardize_wqdata() as well. I assume we would not keep the MDL_UNIT column in the output of wqbc::clean_wqdata().

Can you confirm:

Can you clarify what the wqbc::clean_wqdata() function is doing to the ResultLetter column? I see it retains the column in the output but was not able to identify a place where it modified the contents of the column.

HeatherGranger commented 1 year ago

@aylapear I can confirm that is the workflow. Those MDL_UNIT outputs make sense as well. @KarHarker I'm not sure what/if clean_wqdata does the RESULT_LETTER column.

HeatherGranger commented 1 year ago

ResultLetter column should be retained and not have anything done to it given it gives us the information if the Result is less than detection.