Skip to content

30 new function label converter #33

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

Merged
merged 24 commits into from
Jul 15, 2024

Conversation

SanderDevisscher
Copy link
Collaborator

fixes #30

@SanderDevisscher SanderDevisscher added Function Issue related to a function New Nieuwe functie/data labels Jul 4, 2024
@SanderDevisscher SanderDevisscher self-assigned this Jul 4, 2024
@SanderDevisscher SanderDevisscher linked an issue Jul 4, 2024 that may be closed by this pull request
18 tasks
Copy link
Collaborator

@soriadelva soriadelva left a comment

Choose a reason for hiding this comment

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

The code itself works well but I have a few comments that may be addressed:

  • When labeltype is hardcoded, uppercase letters are not accepted, see:

    #' df <- data.frame(
    #' id = 1:1000,
    #' labelnummer = sample(1:1000, 1000, replace = TRUE),
    #' soort = sample(c("REE", "WILD ZWIJN", "DAMHERT"), 1000, replace = TRUE))
    #'
    #' labels <- label_converter(df, "id", "labelnummer", "soort", "REEKITS", 2020, "eloket")
    which throws an error, because only the lowercase reekits is accepted.

  • The df of the final example cannot be constructed because column labeltype is not present in initial dataset labels, throwing an error on this line:

    #' mutate(labeltype = ifelse(is.na(labeltype), sample(c("REEKITS", "REEGEIT", "REEBOK", NA), 1000, replace = TRUE), labeltype))

  • Not that important, but I was wondering whether it would be better to place the function parameters in English (i.e., labelnumber instead of labelnummer, species_column instead of soort_column,...)

  • Also not that important but it may be nice to add a line after each warning that involves checking, saying that the check passed. e.g., after this warning: 1: In label_converter(df, "id", "labelnummer", "REE", "labeltype", : soort_column: REE is not a column of input >> checking if it's an allowed species there could be a line saying check passed 🎉

@SanderDevisscher SanderDevisscher marked this pull request as draft July 9, 2024 11:41
Copy link
Collaborator

@soriadelva soriadelva left a comment

Choose a reason for hiding this comment

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

When I try to load the function, it seems to get stuck after line 136, not sure what is happening but it may be good to check if this also happens for you @SanderDevisscher

@SanderDevisscher
Copy link
Collaborator Author

@soriadelva how do you mean gets stuck ?

@SanderDevisscher
Copy link
Collaborator Author

The code itself works well but I have a few comments that may be addressed:

  • When labeltype is hardcoded, uppercase letters are not accepted, see:
    #' df <- data.frame(
    #' id = 1:1000,
    #' labelnummer = sample(1:1000, 1000, replace = TRUE),
    #' soort = sample(c("REE", "WILD ZWIJN", "DAMHERT"), 1000, replace = TRUE))
    #'
    #' labels <- label_converter(df, "id", "labelnummer", "soort", "REEKITS", 2020, "eloket")

    which throws an error, because only the lowercase reekits is accepted.

Fixed 🥳

  • The df of the final example cannot be constructed because column labeltype is not present in initial dataset labels, throwing an error on this line:
    #' mutate(labeltype = ifelse(is.na(labeltype), sample(c("REEKITS", "REEGEIT", "REEBOK", NA), 1000, replace = TRUE), labeltype))

Added a remark in the example. You should run an example which results in less than 1000 labels first (e.a. example 1)

  • Not that important, but I was wondering whether it would be better to place the function parameters in English (i.e., labelnumber instead of labelnummer, species_column instead of soort_column,...)

Not implemented since function will be used by us only. I could translate everything to dutch if prefered (as a new issue)

  • Also not that important but it may be nice to add a line after each warning that involves checking, saying that the check passed. e.g., after this warning: 1: In label_converter(df, "id", "labelnummer", "REE", "labeltype", : soort_column: REE is not a column of input >> checking if it's an allowed species there could be a line saying check passed 🎉

Implemented, whenever the function checks something and the check is succesfully completed check passed 🎉 is printed.

Additional fixes and enhancements:

  1. I've implemented some logic to bypass the labeltype logic when labeltype_column is not provided and not needed (only needed with soort = "ree")
  2. I've added a collapsed list of the allowed species and labeltypes to the error message you get when they are not allowed.

@SanderDevisscher SanderDevisscher dismissed soriadelva’s stale review July 9, 2024 18:48

changes implemented

@SanderDevisscher SanderDevisscher marked this pull request as ready for review July 9, 2024 18:48
@soriadelva
Copy link
Collaborator

@soriadelva how do you mean gets stuck ?

it's weird, when I just run the function in the code itself it keeps on getting stuck at that same line, like this:
stuck
But if I select the whole code with ctrl + A and then run it, it seems to be fine 🤔

@SanderDevisscher SanderDevisscher merged commit 38a29d8 into main Jul 15, 2024
6 checks passed
@SanderDevisscher SanderDevisscher deleted the 30-new-function-label_converter branch July 15, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Function Issue related to a function New Nieuwe functie/data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NEW function] label_converter
2 participants