Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

284 add support for camtrap dp v1.0 #286

Merged
merged 39 commits into from
Nov 8, 2023
Merged

Conversation

PietrH
Copy link
Member

@PietrH PietrH commented Nov 3, 2023

STATUS: Ready for review

  • Check that all mentions of the release candidate are removed and replaced by v1.0

Failing tests:

test-read_camptrap_dp.R

  • deployments.baitUse is expected to be NA but is actually none, it should be NA since it's false in the source, these values have changed between the examples in the rc and v1.0
  • deployments.session is expected to be NA but has values from deploymentGroups, these values have changed between the examples in the rc and v1.0

Missing tests

I feel additional coverage was needed for the changes below, since we were passing tests for a new v1.0 camtrap-dp even though they were not implemented yet.

Add tests for:

  • Downconversion of media.captureMethod values activityDetection to motionDetection
  • Downconversion of project.catureMethod (in metadata) values activityDetection to motionDetection

@PietrH PietrH self-assigned this Nov 3, 2023
@PietrH PietrH linked an issue Nov 3, 2023 that may be closed by this pull request
7 tasks
@PietrH PietrH force-pushed the 284-add-support-for-camtrap-dp-10 branch from 4c26871 to 5fda125 Compare November 3, 2023 13:48
@PietrH PietrH force-pushed the 284-add-support-for-camtrap-dp-10 branch from 03edb2a to 0615366 Compare November 3, 2023 13:54
@PietrH PietrH changed the title 284 add support for camtrap dp 10 284 add support for camtrap dp v1.0 Nov 3, 2023
@PietrH
Copy link
Member Author

PietrH commented Nov 3, 2023

I seem to have pulled some code from #282 into test-read_camtrap_dp.R by accident, I'll try to remove anything that might result in merge conflicts with #282

@PietrH PietrH force-pushed the 284-add-support-for-camtrap-dp-10 branch from 2dbbf12 to 391c027 Compare November 3, 2023 14:26
@PietrH PietrH force-pushed the 284-add-support-for-camtrap-dp-10 branch from ab187d6 to 8a31e59 Compare November 3, 2023 14:44
@PietrH
Copy link
Member Author

PietrH commented Nov 6, 2023

I would just use the package as is and remove some of the tests. They are only used to test the downconversion, which will disappear at some point.

I'd rather skip the test for now, so it's easier to debug if it does happen to break before we switch to the different data model in camtraptor. Skipping implemented in fb477ef

@PietrH PietrH added the urgent label Nov 6, 2023
@PietrH
Copy link
Member Author

PietrH commented Nov 6, 2023

@peterdesmet Should deployments.deploymentGroups be mapped to deployments.session in v0.1.6 ?

This is how it works at the moment;

  if ("deploymentGroups" %in% names(deployments)) {
    # map to session and then remove
    deployments <- deployments %>%
      dplyr::mutate(session = dplyr::case_when(
        is.na(.data$session) ~.data$deploymentGroups,
        is.na(.data$deploymentGroups) ~ .data$session,
        !is.na(.data$deploymentGroups) & !is.na(.data$session) ~ 
          stringr::str_c(.data$session, 
                         .data$deploymentGroups, 
                         sep = " | "))) %>%
      dplyr::select(-"deploymentGroups")
  }
  

If there is no value for session, it becomes deploymentGroups, if there is no value for deploymentGroups, it become session, if there is a value for both, they are concatenated with a pipe.

@peterdesmet
Copy link
Member

Regarding deploymentGroups: functionality is good, but would simplify and reverse order (so that deploymentGroup info is always kept):

  1. Both present: concatenate with pipe
  2. deploymentGroups present (and not session): populate session
  3. session present: keep in session

Remove deploymentGroups afterwards

@PietrH PietrH marked this pull request as ready for review November 6, 2023 13:30
@PietrH
Copy link
Member Author

PietrH commented Nov 6, 2023

Ready for review, something is going wrong building the vignettes, any ideas?

R/zzz.R Outdated Show resolved Hide resolved
R/zzz.R Outdated Show resolved Hide resolved
R/zzz.R Outdated Show resolved Hide resolved
@damianooldoni
Copy link
Member

I couldn't reproduce the error while building the vignette with windows-latest+ R 3.6.0.
It's a problem while generating the "black multiply" icon in leaflet.
I am afraid that leaflet is not really concerned about supporting older R versions on all type of machines. See https://github.com/rstudio/leaflet/actions/runs/6776222357

They support R 3.6.3 on Ubuntu only. Everything runs on Ubuntu except a machine running R 4.3.2 on windows-latest.
My proposal is to remove such check as well and so leaving only the check with Ubuntu + R 3.6.3.

@PietrH
Copy link
Member Author

PietrH commented Nov 8, 2023

Ok for me

@peterdesmet
Copy link
Member

Fine by me. Can we merge this request? Note that it contains a version bump to v0.21.0 which you may want to include only after you fixed other issues.

@damianooldoni
Copy link
Member

To avoid too much confusion with versioning, I think we should just merge this branch to main as version 0.21.0 asap.
I would then leave the version number as it is for the next imminent PRs and then a release will follow.

@PietrH PietrH merged commit 166e246 into main Nov 8, 2023
8 checks passed
@PietrH PietrH deleted the 284-add-support-for-camtrap-dp-10 branch November 8, 2023 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Camtrap DP 1.0
3 participants