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

Negative S_PIE values when very small non integer abundances #258

Closed
AlbanSagouis opened this issue Dec 9, 2020 · 11 comments
Closed

Negative S_PIE values when very small non integer abundances #258

AlbanSagouis opened this issue Dec 9, 2020 · 11 comments

Comments

@AlbanSagouis
Copy link
Contributor

When the abundance matrix mostly has small non integer values (because of standardisation), S_PIE is negative:

> library(mobr) # recently installed from the dev branch 
> set.seed(42)
> calc_div(runif(100, 0, 1.3), 'S_PIE', scales = 'alpha')
[1] -711.2691

Printing a warning could help draw attention to these communities or the functioning of S_PIE?

@dmcglinn
Copy link
Member

dmcglinn commented Dec 9, 2020

Thanks for noting this @AlbanSagouis ! The function that needs to be changed is calc_PIE which @T-Engel and I have been working on in his branch (note to see comments here #254 and here 48224b3 ). But it looks like this may be something we want to think about adding. The package as currently implemented is only designed for integer abundance values. If we decide to extend it to non-integer abundances or relative abundances than we will want to change several different functions. So far that has been a lower priority than actually extending the package to presence-absence. @jmchase do you see a big immediate need to accommodate non-integer abundances?

@dmcglinn
Copy link
Member

dmcglinn commented Dec 9, 2020

But I should note at a minimum if users are trying to use non-integer abundances then we should give them a warning since we don't currently support that.

@dmcglinn dmcglinn added ToDo and removed enhancement labels Dec 9, 2020
@jmchase
Copy link

jmchase commented Dec 9, 2020

Yes, there are several cases where people want to use non-integer abundances. Even when it's not percent cover. For example, if averages are taken beforehand. Not ideal, I know. But often the field worker thinks it's the right thing to do, for standardization, etc. So, while it's not a priority per se, we need to deal with it.

When Felix dealt with this for our fragmentation paper, we 1) rounded up to nearest integer, 2) used rarefaction anyway and it worked (except not for Spie clearly). I think we need to give some advice. My gut feeling is 'nearest integer' works fine, and maybe should make that a default?

@T-Engel
Copy link
Contributor

T-Engel commented Dec 9, 2020

Thanks, Alban. In my opinion, we should return an error when this happens (in all functions). If a user thinks it's a good decision to round the data to integers, they can do that. But I wouldn't want to do it for them by default when I can't be sure what the data represent.

@T-Engel
Copy link
Contributor

T-Engel commented Dec 9, 2020

I implemented the changes we discussed here: 4114e96

So, now there is a new function calc_SPIE that can be used to calculate SPIE, while calc_PIE should only be used to calculate PIE. This means calc_PIE no longer has the ENS argument.
Instead, we added a new argument "replace" to both functions, which lets you switch between standardized (i.e. replace=F, the default) and unstandardized (replace= T) versions of PIE and SPIE.
Now there is an error message when the data are not integers values.
And there are warnings when NAs are produced. @sablowes

Please, try it out and tell me if it behaves as expected.

@dmcglinn
Copy link
Member

Hey @T-Engel I like your changes but we still had that pesky problem of PIE getting set to 1 when no individuals were in the sample so I fixed that here: 242625e

Also I made other small changes to the code style and documentation. Has anyone published PIE values we can test against? Otherwise we need to put together an example we've computed by hand to run tests against. I can't stress how important a test for PIE is for the package. We have tests for the rarefaction piece though: https://github.com/MoBiodiv/mobr/blob/master/tests/testthat/test_rarefaction.R

@AlbanSagouis
Copy link
Contributor Author

Thanks! calc_div(index = 'S_PIE') error message makes total sense!

Error in calc_PIE(x, ENS = TRUE) : 
  The ENS argumet was removed from this function. Please, use calc_SPIE() for the ENS transformation of PIE. 

T-Engel added a commit that referenced this issue Dec 10, 2020
- both functions get the replace argument
- use calc_SPIE when index="S_PIE
- documentation

#258 (comment)
@T-Engel
Copy link
Contributor

T-Engel commented Dec 10, 2020

Thanks for catching that, Dan! Yes a test would be good. Do you think we could just compare it to vegan?

@AlbanSagouis. Oh yeah, that makes sense too. This is because we hadn't updated the rest of the code. I just changed calc_div() and calc_comm_div() here 8f479b2. Now they should work too.

@dmcglinn
Copy link
Member

Looks great @T-Engel I made only superficial fixes: c5fd569

@T-Engel
Copy link
Contributor

T-Engel commented Dec 10, 2020

Thanks, Dan. I promise I will spell out TRUE and FALSE in future 😅

@dmcglinn
Copy link
Member

dmcglinn commented Dec 10, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants