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

Finalize n_species, deprecate/defunct other functions, add CITATION.cff #346

Merged
merged 34 commits into from
Dec 24, 2024

Conversation

damianooldoni
Copy link
Member

@damianooldoni damianooldoni commented Nov 20, 2024

This pull request includes several changes to the codebase, focusing on:

  • finalizing n_species() and its tests
  • deprecating some functions
  • improving documentation

Finalizing n_species() and its tests

Up to now, tests were not really taken into account as we were way far from getting the functions ready to the new format. From this PR we can start using them 🥳 as the very first function, n_species(), is ready! So, please run devtools::test(filter = "n_species") as check. Its related tests have been simplified without losing testing power. Notice that we only consider event based observations for the standard functions of camtraptor v1.0.0 (#332).

Deprecation of functions

Metadata

Documentation updates

  • Updated references in multiple R files to inherit arg description from n_species instead of the deprecated get_species. This has been done by using @inheritParams n_species instead of @inheritParams get_n_species.
  • Updated the NEWS.md file to reflect the deprecation/defunct of several functions, the introduction of new replacements and new citation file.

Other code improvements

  • Simplified get_n_individuals() and get_n_obs() functions by removing the species parameter and associated logic: we do not filter anymore within such functions, only via filter_observations(). IMPORTANT: these functions are still not finalized and do not follow the new data standard still. So, they will fail or return a non tested result.

@damianooldoni damianooldoni self-assigned this Nov 20, 2024
@damianooldoni damianooldoni marked this pull request as ready for review November 20, 2024 16:36
Copy link
Member

@sannegovaert sannegovaert left a comment

Choose a reason for hiding this comment

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

Nice!
I have 1 remark about filtering on observationLevel & a suggestion for simplification.

R/n_species.R Outdated
# Extract observations and deployments
observations <- observations(x)
# Extract event-based observations and deployments
observations <- observations(x) %>%
Copy link
Member

Choose a reason for hiding this comment

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

I suggest:
x <- filter_observations(.data$observationLevel == "event")
and using observations(x) instead of observations.

Because what if there is a deployment with only media-based observations?
It would now not be noticed in L33: deployments_no_obs <- get_dep_no_obs(x).

And last, why is the count reduced by NA-values in a mutate-function, and not like this?:

n_species <-
   species %>% 
   dplyr::filter(!is.na(.data$scientificName)) %>% 
   dplyr::group_by(.data$deploymentID) %>%
   dplyr::count() %>%
   dplyr::ungroup()

Copy link
Member Author

Choose a reason for hiding this comment

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

Fully agree with first suggestion 🔝 Implemented in 28ae966 and 328a573.
About the reason why I use a mutate to not count NA as species: I agreed with you at the beginning , but then thanks to a failing test (A perfect example of the need of unit-tests!) I found the reason I wrote that few years ago 😄 We need to assign 0 to deployments with ONLY unidentified species (scientificName = NA). To do that, the shortest way I found was to do a standard group_by() %>% count() and then reducing the sum by one. I improved the comments in the code to better understand this for future-me and everybody else.

@damianooldoni
Copy link
Member Author

I also ordered the output by using the original order as provided in deployment table. Tests added to check this.

Also added a test to check that deployments without event-based observations have n = NA as for deployments without any observation at all.

Copy link
Member

@sannegovaert sannegovaert left a comment

Choose a reason for hiding this comment

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

Well done, I have no comments!

@damianooldoni damianooldoni merged commit 32db4b8 into version-1.0 Dec 24, 2024
@damianooldoni damianooldoni deleted the species_functions branch December 24, 2024 09:11
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.

Add CITATION.cff Deprecate get_species() Remove check_species(), get_scientific_name()
2 participants