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

Adapt get_custom_effort #335

Merged
merged 12 commits into from
Oct 14, 2024
Merged

Adapt get_custom_effort #335

merged 12 commits into from
Oct 14, 2024

Conversation

damianooldoni
Copy link
Member

@damianooldoni damianooldoni commented Sep 19, 2024

This PR will solve #333.

Thanks @MartijnUH for the very well description of the requested feature and the code.

I think I adapted get_custom_effort as you wished.

Test it hard please 🔨

Can you please test the PR and so review it? Thanks.
You can install this PR directly in R:

devtools::install_github("inbo/camtraptor", ref = "pull/335/head")

Notice that in the example on how to calculate the total effort over all deployments (added as example in function documentation), I needed to add dplyr::filter(effort > 0) to your provided code, otherwise I get all deployments in deploymentIDs, ndep and nloc:

get_custom_effort(mica, group_by = "year", unit = "day") %>%
  dplyr::filter(effort > 0) %>%
  dplyr::group_by(begin) %>% 
  dplyr::summarise(
    deploymentIDs = list(deploymentID),
    locationNames = list(locationName),
    ndep = length(unique(deploymentID)),
    nloc = length(unique(locationName)),
    effort = sum(effort),
    unit = unique(unit)
  )

@MartijnUH
Copy link
Collaborator

MartijnUH commented Sep 27, 2024

@damianooldoni see review below:

Copy link
Collaborator

@MartijnUH MartijnUH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include a new assert_that to check for NAs in package$data$deployments$start and package$data$deployments$end to prevent the uniformative error message: Error in seq.int(0, to0 - from, by) : 'to' must be a finite number. See comment on R95.

"Did you forget to convert a string to Date with `as.Date()`?"
)
)

# Define possible unit values
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@damianooldoni the function seems to behave as intended. The only minor issue that I have found is that the function returns an uninformative error message (i.e. Error in seq.int(0, to0 - from, by) : 'to' must be a finite number) when either start or end columns of package$data$deployments contains NAs. This could be improved by returning a more informative error message.

I suggest that after these first assert_that checks, another check should evaluate if the start and end columns of the deployments do not contain NAs:

  • include new assert_that to check for NAs in package$data$deployments$start and package$data$deployments$end

@damianooldoni
Copy link
Member Author

Thanks @MartijnUH. I will check it. I also got an email so I could find your comment there 👍
image

@damianooldoni damianooldoni merged commit 079713f into main Oct 14, 2024
8 checks passed
@damianooldoni damianooldoni deleted the adapt-get_custom_effort branch October 15, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants