BiologicalRecordsCentre / sparta

Species Presence/Absence R Trends Analyses
http://biologicalrecordscentre.github.io/sparta/index.html
MIT License
21 stars 24 forks source link

New function summaryVis() #186

Closed galinajonsson closed 4 years ago

galinajonsson commented 4 years ago

This function takes a formatOccData object and returns a dataframe summarising the percentage of total sites that have been visited twice or more (i.e. revisited) within each closure period, as well as the mean number of visits to sites that have been visited two times or more for each closure period.

AugustT commented 4 years ago

Can you rename this function visitsSummary? the abbreviation 'vis' is often used to mean visualisation, so it is a bit ambiguous. Kicking off the tests again, not sure the failure was your fault.

codecov-io commented 4 years ago

Codecov Report

Merging #186 into master will increase coverage by 0.19%. The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
+ Coverage   58.71%   58.91%   +0.19%     
==========================================
  Files          83       84       +1     
  Lines        2638     2653      +15     
==========================================
+ Hits         1549     1563      +14     
- Misses       1089     1090       +1
Impacted Files Coverage Δ
R/errorChecks.r 97.65% <ø> (ø) :arrow_up:
R/visitsSummary.R 93.33% <93.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 996d011...d8af374. Read the comment docs.

AugustT commented 4 years ago

Checks pass. The complaint it is making is that you have not added a test for this function. Have you written tests before @galinajonsson ? If you are happy to do that I can wait for you to add a test, if you aren't I can merge this in and add an issue to build a test at a later date.

galinajonsson commented 4 years ago

Hi @AugustT. I've changed the name to visitSummary, added a test and have pushed these to my master branch.

AugustT commented 4 years ago

It looks like your new tests are failing because the results don't quite match. Do you see this when you run it on your machine?

── 1. Failure: Test formatOccData errors (@testvisitsSummary.R#28)  ────────────
`visitData <- formatOccData(taxa = taxa, site = site, survey = survey)` produced unexpected warnings.
Expected match: 859 out of 15000 observations will be removed as duplicates
Actual values:
* 828 out of 15000 observations will be removed as duplicates
── 2. Failure: Test all columns present (@testvisitsSummary.R#37)  ─────────────
`visitData <- formatOccData(taxa = taxa, site = site, survey = survey)` produced unexpected warnings.
Expected match: 859 out of 15000 observations will be removed as duplicates
Actual values:
* 828 out of 15000 observations will be removed as duplicates
── 3. Failure: Test formatOccData outputs (@testvisitsSummary.R#46)  ───────────
`visitData <- formatOccData(taxa = taxa, site = site, survey = survey)` produced unexpected warnings.
Expected match: 859 out of 15000 observations will be removed as duplicates
Actual values:
* 828 out of 15000 observations will be removed as duplicates
── 4. Failure: Test formatOccData outputs (@testvisitsSummary.R#53)  ───────────
head(visitsSum) not identical to `head_visitSum`.
Component "PercRevisited": Mean relative difference: 0.06122449
Component "meanRevisits": Mean relative difference: 0.008314437
══ testthat results  ═══════════════════════════════════════════════════════════
[ OK: 268 | SKIPPED: 0 | WARNINGS: 0 | FAILED: 4 ]
1. Failure: Test formatOccData errors (@testvisitsSummary.R#28) 
2. Failure: Test all columns present (@testvisitsSummary.R#37) 
3. Failure: Test formatOccData outputs (@testvisitsSummary.R#46) 
4. Failure: Test formatOccData outputs (@testvisitsSummary.R#53) 
galinajonsson commented 4 years ago

How odd! It looks like I get different set.seed values on my machine (I double checked with other tests). It might be an R version issue (I'm using 3.5.2). I've changed the expected values to those of the travis CI check and for the time being, commented out one test that uses actual output values.

AugustT commented 4 years ago

Your right, R did change the random number seed. It destroyed all my tests!