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

update limits and codes #117

Closed sebdalgarno closed 4 years ago

sebdalgarno commented 4 years ago

I have updated the limits and codes table in wqbc and have had to update some tests as well to test new values (from adjusted Limits) and removed variables (Copper Total, Arsenic Total, etc).

The updated limits are a simplified version of the new guidelines spreadsheet developed by Angeline Tillmans and co. Only limits with Use = "Freshwater Life" and Media = "Water" have been kept so as to require minimal changes in the actual calc_limits function code.

Note more limits (for different users/media etc.) might be added next year but for now we wanted to provide the basic functionality for some new/expanded limits.

To see the whole new guidelines data:

limits_new <- bcdata::bcdc_get_data(record = "85d3990a-ec0a-4436-8ebd-150de3ba0747")

fixes #114

ateucher commented 4 years ago

One question: why not just fully replace the old limits.csv with the processed data from the catalogue dataset, rather than merging/conditionally replacing parts?

sebdalgarno commented 4 years ago

none of the old limits have been moved into the new limits table. The section where a series of tmp objects are created (e.g. tmp <- anti_join(limits, limits_new, c("EMS_Code", "Term")) is simply me confirming to myself (and anyone interested) that there is a reason for each old limit missing in the new limit table.

I have bound any missing old codes to the new codes table, and these could probably be removed (ie. just create new codes table from the new limits table)

Does that answer your question?

sebdalgarno commented 4 years ago

I'm going to get Angeline Tillmanns to update the guidelines spreadsheet on DataBC as it is getting to a point of stability now. I will then update the limits data again and clean up the 'limits-update.R' script. I think this will help to remove some of the cleaning required and other things in consideration of your comment: "One question: why not just fully replace the old limits.csv with the processed data from the catalogue dataset, rather than merging/conditionally replacing parts?"

ateucher commented 4 years ago

Makes sense @sebdalgarno - would you prefer I merge this now so you can move along with your other plans, then you can submit a new PR once the guidelines are updated in the data catalogue? Or do you want to hold off and do it all in one go?

sebdalgarno commented 4 years ago

thanks @ateucher. Angeline just updated the guidelines. I will push the updates this morning and you can merge in one go.

sebdalgarno commented 4 years ago

OK @ateucher ready for final review and merge

sebdalgarno commented 4 years ago

OK @ateucher thanks for your patience on this one and for the good review. I've now pushed changes that address all of your comments/suggestions above, including renaming limits-update.R to limits.R

ateucher commented 4 years ago

Looking good @sebdalgarno, thanks! Can you now delete limits-update.R?

ateucher commented 4 years ago

Thanks! I'm going to merge this, then push up a couple of local changes I made to some of the other data-raw scripts.

sebdalgarno commented 4 years ago

OK thanks Andy! that wraps up all the PRs I was planning on making to wqbc and rems. I think @joethorley is working on a PR for wqbc (on censored data)