Fix 288 and 279 #289

damianooldoni · 2023-11-08T15:58:29Z

This PR solves:

add number of individuals to output of get_record_table #279
Extend functionality of get_cam_op: allow sessionCol argument to add sessionID in stationName column as in camtrapR #288

Also, some examples of get_record_table have been updated as the last version of mica datapackage we are using do not contain observations made in quick succession. Same holds true for the correspondent unit-tests.

The new mica datapackage example requires some changes to make meaningful example of setting minDeltaTime to remove not independent observations.

Column added in 201737e

Multiply by ten actually or in character terms adding a zero

Same as f37ef07

Examples updated as well

damianooldoni · 2023-11-09T14:32:04Z

@MartijnUH: could you please check the two functions in your use cases?
@PietrH: you can maybe start review or waiting until automatic checks are :green

Co-Authored-By: Peter Desmet <[email protected]>

… ...`

PietrH · 2023-11-10T09:41:05Z

camtraptor/tests/testthat/test-get_cam_op.R

Lines 74 to 77 in 3af4488

    
           test_that("output is a matrix", { 
        
             cam_op_matrix <- get_cam_op(mica) 
        
             expect_true(is.matrix(cam_op_matrix)) 
        
           })

It's a real shame testthat has no real expectation for matrices, expect_type() can't handle it, and since it's not a S3 or S4 object, neither can expect_s3_class() or expect_s4_class()

easier to read and slight performance benefit

PietrH

I've left some questions out of interest for @damianooldoni to have a look at when it suits him. Any changes I felt necessary I've made myself. I looked at the functionality as best as I could, but will depend on @MartijnUH to check if it suits the teams needs.

Great job Damiano!

R/get_record_table.R

PietrH · 2023-11-10T08:51:29Z

tests/testthat/test-get_record_table.R

+  record_table <- get_record_table(mica)
+  expected_colnames <- c("Station",
+                         "Species",
+                         "n",
+                          "DateTimeOriginal",
+                         "Date",
+                         "Time",
+                         "delta.time.secs",
+                         "delta.time.mins",
+                         "delta.time.hours",
+                         "delta.time.days",
+                         "Directory",
+                         "FileName"
+  )
+  testthat::expect_identical(names(record_table), expected_colnames)


Suggested change

record_table <- get_record_table(mica)

expected_colnames <- c("Station",

"Species",

"n",

"DateTimeOriginal",

"Date",

"Time",

"delta.time.secs",

"delta.time.mins",

"delta.time.hours",

"delta.time.days",

"Directory",

"FileName"

)

testthat::expect_identical(names(record_table), expected_colnames)

expect_named(

get_record_table(mica),

c(

"Station",

"Species",

"n",

"DateTimeOriginal",

"Date",

"Time",

"delta.time.secs",

"delta.time.mins",

"delta.time.hours",

"delta.time.days",

"Directory",

"FileName"

)

)

PietrH · 2023-11-10T08:54:10Z

tests/testthat/test-get_record_table.R

+    deltaTimeComparedTo = "lastRecord"
+  )) %>%
+    nrow()
+  testthat::expect_true(nrow_delta_10000 < nrow_delta_0)


Did you know about:

testthat::expect_lt()

These comparison expectations have slightly nicer failure messages

https://testthat.r-lib.org/reference/comparison-expectations.html

PietrH · 2023-11-10T08:56:57Z

tests/testthat/test-get_record_table.R

+  mica_dup <- mica
+  # create duplicates at 2020-07-29 05:46:48, location: B_DL_val 5_beek kleine vijver
+  # use 3rd observation as the first two are unknown or blank (= no animal)
+  mica_dup$data$observations[,"sequenceID"] <- mica_dup$data$observations$sequenceID[3]


I would use purrr::chuck for this, because I find it more readable. But it's a matter of preference. Good job documenting by the way!

purrr::pluck is careful, and will fail silently, purrr::chuck will trow (chuck) an error when the index doesn't exist, another advantage.

PietrH · 2023-11-10T08:59:05Z

vignettes/record-table.Rmd

-get_record_table(mica, 
-                 minDeltaTime = 60, 
+mica_dependent <- mica
+mica_dependent$data$observations[4,"timestamp"] <- lubridate::as_datetime("2020-07-29 05:55:00")


I tend to use lubridate::dmy_hms() for character vectors, is there an advantage to lubridate::as_datetime()?

PietrH · 2023-11-10T09:00:02Z

vignettes/camera-operation-matrix.Rmd

+
+### Session and camera IDs
+
+You can specify the column containing the camera IDs to be added to the station names following the camtrapR's convention: `Station__CAM_CameraID`. Only the row names are shown:


Why only show the row names?

PietrH · 2023-11-10T09:44:34Z

R/get_cam_op.R

+        "it must be one of the deployments column names."
+      )
+    )
+    camera_values <- package$data$deployments[[camera_col]]


I'd personally use purrr::chuck() for this so I throw an error when I try to access an index that doesn't exist. I'm not really sure what the base behaviour is in this case. Ok to leave like this.

PietrH · 2023-11-10T10:35:05Z

tests/testthat/test-get_record_table.R

    output <- output %>%
      dplyr::left_join(n_media,
        by = "sequenceID"
      )
-    testthat::expect_equal(output$len, output$n)
+    testthat::expect_equal(output$len, output$n_media)


n_media is a integer vector, and len is a double vector. Is this intentional?

damianooldoni added 22 commits November 8, 2023 16:45

Fix #279

201737e

Add test about expected columns returned

d58f547

Creates meaningful examples of using minDeltaTime

22eda44

The new mica datapackage example requires some changes to make meaningful example of setting minDeltaTime to remove not independent observations.

Update species in example comment

3dbd53b

Update a test after adding column n

f2aabf5

Column added in 201737e

Adjust threshold for removing not independent obs

88f7641

Multiply by ten actually or in character terms adding a zero

Add line in news about solving #279

7be7555

Add new column to vignette

553bf1c

Add examples about duplicate removel in examples

2710a3c

Update chunks about duplicates based on new mica dataset

fc60b6f

Use mica_dup instead of mica_duplicates

f37ef07

Explain why using 3rd obs as template for mica_dup

d8dc125

Use 3rd timestamp in test

c2b320f

Use mica_dup instead of mica_duplicates

1ea3aea

Same as f37ef07

Fix #288

ba7eb71

Examples updated as well

Run devtools::document

0c01da0

Add check reserved words in station column

6e1fef4

Remove NAs before checking presence reserved words

9e101f8

Add tests of new features

d208521

Update vignette describing new features

9f5d484

Report new features of get_cam_op in NEWS.md

2a1181e

Remove typo

3870869

damianooldoni marked this pull request as ready for review November 9, 2023 14:22

damianooldoni mentioned this pull request Nov 9, 2023

Extend functionality of get_cam_op: allow sessionCol argument to add sessionID in stationName column as in camtrapR #288

Closed

Load dplyr in examples to use %>%

ac380cd

damianooldoni requested review from PietrH and MartijnUH November 9, 2023 14:29

damianooldoni added 2 commits November 9, 2023 15:48

Add importFrom for rlang symbols !! and :=

a593839

Run devtools::document()

bf09332

Improve grammar

8eccfa2

Co-Authored-By: Peter Desmet <[email protected]>

damianooldoni mentioned this pull request Nov 9, 2023

Inherit parsing issues #282

Merged

PietrH added 4 commits November 10, 2023 10:13

use testthat::expect_named() instead of `expect_identical(colnames(…

5a81082

… ...`

add missing space in documentation

55d1eba

use is.matrix() for a slightly better failure message

68f591c

Mention the specific argument needed to specify

3af4488

Reduce numerical tolerance, increase strictness

e7d99ad

PietrH force-pushed the fix-288-and-279 branch from 94fad6b to e7d99ad Compare November 10, 2023 10:06

use comparison expectations for better failure messages

a31614d

PietrH force-pushed the fix-288-and-279 branch from 9a81e70 to a31614d Compare November 10, 2023 10:32

PietrH added 2 commits November 10, 2023 11:37

increase test strictness by removing tolerance

4dcdd65

No need to announce testthat namespace in tests

c264eba

easier to read and slight performance benefit

PietrH approved these changes Nov 10, 2023

View reviewed changes

clarify default behaviour

41afe9a

PietrH merged commit cee8f65 into main Nov 10, 2023
8 checks passed

PietrH deleted the fix-288-and-279 branch November 10, 2023 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 288 and 279 #289

Fix 288 and 279 #289

damianooldoni commented Nov 8, 2023 •

edited

Loading

damianooldoni commented Nov 9, 2023

PietrH commented Nov 10, 2023

PietrH left a comment

PietrH Nov 10, 2023

PietrH Nov 10, 2023

PietrH Nov 10, 2023

PietrH Nov 10, 2023

PietrH Nov 10, 2023

PietrH Nov 10, 2023

PietrH Nov 10, 2023


		### Session and camera IDs

		You can specify the column containing the camera IDs to be added to the station names following the camtrapR's convention: `Station__CAM_CameraID`. Only the row names are shown:

Fix 288 and 279 #289

Fix 288 and 279 #289

Conversation

damianooldoni commented Nov 8, 2023 • edited Loading

damianooldoni commented Nov 9, 2023

PietrH commented Nov 10, 2023

PietrH left a comment

Choose a reason for hiding this comment

PietrH Nov 10, 2023

Choose a reason for hiding this comment

PietrH Nov 10, 2023

Choose a reason for hiding this comment

PietrH Nov 10, 2023

Choose a reason for hiding this comment

PietrH Nov 10, 2023

Choose a reason for hiding this comment

PietrH Nov 10, 2023

Choose a reason for hiding this comment

PietrH Nov 10, 2023

Choose a reason for hiding this comment

PietrH Nov 10, 2023

Choose a reason for hiding this comment

damianooldoni commented Nov 8, 2023 •

edited

Loading