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

Warn about non-integers in removal() #60

Closed
droglenc opened this issue Apr 9, 2020 · 2 comments · Fixed by #67
Closed

Warn about non-integers in removal() #60

droglenc opened this issue Apr 9, 2020 · 2 comments · Fixed by #67
Assignees

Comments

@droglenc
Copy link
Contributor

droglenc commented Apr 9, 2020

The removal() function is primarily used with catch of individuals data. Thus, the data should generally be whole numbers that are positive. Consider adding a check for whole numbers and throwing a warning, not an error (as some have used removal() to estimate biomass and thus have used non-whole numbers). Do not use is.integer(). Look at is.whole() in several other packages.

@droglenc droglenc self-assigned this Apr 9, 2020
@jcdoll79
Copy link
Member

is.wholenumber() in bazar and is.whole() in bgr check for numeric when checking for whole number. Suggest doing the same.
Submitting the next two lines to FSA:
ct3=c("one","two","three")
removal(ct3)
returns "Error in sum(ct3) : invalid 'type' (character) of arguments.
The error is being thrown on line 254 of removal.R during internal calculations of intermediate values
Suggest adding a check for numeric when checking for integer.
Return error when non-numeric value found and warning when numeric but not whole number.

@droglenc
Copy link
Contributor Author

Your ct3 example brings up several issues with the "checks" at the beginning of removal(). Some thoughts about how to proceed ...

  • The use of as.numeric() when converting a one row or one column matrix or a one column data.frame for use is problematic. For example, as.numeric(data.frame(words=c("one","two","three"))) will throw a "cannot be coerced" error. I think that the the matrix correction (current line 212) should condition on if it is only one row or one column and if it is a row then do catch <- catch[1,] and if it is a row then do catch <- catch[,1]. For the data.frame correction (current line 214) we should use catch <- catch[[,1]].
  • Once it is clear that we are working with a vector (the above bullet) then I think we should check if it is numeric or an integer. I think we should do this here instead of as part of checking if it is a whole number so that we can send a separate warning message like "removal requires numeric data."
  • Once we know it is numeric then we can check for whole numbers and issue a warning if the vector is not all whole numbers.
    • As a general rule, I try to avoid using other packages as much as possible (some "complaints" that I got early on was that FSA was "bloated" because too many other packages were loaded with FSA). In this vein, I think we should make an internal function called iIsWhole() (will be in the FSAInternals.R file that is the code for is.wholeNumber() in the documentation for is.integer(). We can then use that function here. I think we can make it internal because the world does not need another whole number function.
  • Then deal with NAs and catches of 1 as in the current code.

Relatedly ...

  • I think we should move the Tmult= check right below where conf.level= is checked and thus above where the checking of catch starts. This way the simple things that will kill the function appear first, then we get into the more involved checking of catch.
  • I think we should create an internal function that checks the conf.level= and this can be shared across several other functions. See internal function to check conf.level= argument #66.

@jcdoll79 jcdoll79 mentioned this issue Feb 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants