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

Summarize bug #169

Closed aylapear closed 1 year ago

aylapear commented 1 year ago
aylapear commented 1 year ago

This will close #163 and is tied to https://github.com/bcgov/shinyrems/issues/139

joethorley commented 1 year ago

This looks great to me - I approve. Also @aylapear you should add yourself as a ctb.

KarHarker commented 1 year ago

Hi @aylapear - I was wondering if you could grant me temporary access to test your fork? I have a few WQ scripts I like to run as an extra test. If not I can pull first and then let you know if I run into anything.

aylapear commented 1 year ago

Hi @KarHarker, there are two different ways you can access the code and install the package. Pick which option works for you better.

  1. Run this line of code and it will install the package with the changes: remotes::install_github("poissonconsulting/wqbc@summarize_bug")

  2. Go to the forked repo (https://github.com/poissonconsulting/wqbc), clone the repo, once on your computer switch to the summarize_bug branch and then build the package.

The forked repo is public so you should be able to fully access it. Let me know if you have any issues.

KarHarker commented 1 year ago

Hi @aylapear - thanks. I was getting weird permissions errors when I was trying to clone it earlier so I thought the fork was private. But I got it sorted.

Your changes look good to me! One small thing - I am wondering about modifying the ncen column so that is shows the number of censored observations regardless of whether the user opts for the ml summary stats estimation. Right now when you use summarise_wqdata(x, censored=FALSE) the ncen automatically defaults to 0. I think it makes more sense to still provide the ncen as the numbers of observations where the Value <= Detection Limit. @HeatherGranger any thoughts?

HeatherGranger commented 1 year ago

From what I understand what you're saying @KarHarker makes sense.