pre-processing for mixture3p and IMM model fails if the setsize variable contains character information

[GidonFrischkorn]

When I tested the mixture3p model for the Vignette on the mixture models using the Bayes data set from the mixtur package, I noticed that the preprocessing and creating of the Index variables fails, because the variable is coded as Set Size 1, Set Size 2, Set Size 4, and Set Size 6.

We should include a notification that the setsize variable should be coded either numerically, if it is entered as a continuous predictor, or with factor labels that only use single digits. Alternatively, we could see if we can remove all alphabet letters from the set size labels to only maintain the digit information. But this would also fail, if someone uses labels such as "one", "two", "three", etc. for coding a set size factor.

[GidonFrischkorn]

if we want to remove the letters from the labels we could use: gsub('[[:alpha:] ]', '', labels)

[venpopov]

good catch. I think best to keep it simply. Requere it to be either numeric, or coercible to numeric (digits as factors). I'll add that as a stop condition and test

[venpopov]

Wow, my bad, I recoded it that way to have nice facet labels in the plots, and since I never ran the mixture3p model, just wrote "it's the same as the 2p". That'll teach me!

Anyway, positive thing at the end, because we can catch this behavior.

[venpopov]

Now that I tested it, this is even a bigger issue - if you have a factor with labels such as "Set size 1", "...2", "...4" and "...8", the current code would translate it as set size 1,2,3, and 4 and never give an error. The model would have run, and the results would have been plausible, but still wrong!

Update: actually it wouldn't. It would just throw the same error you encountered. Not sure what I was referring to earlier, since the old code used as.numeric(as.character(...)). It would still return an NA vector causing problems down the line without identification what caused the issue, so has to be fixed anyway. But not as dramatic as I pointed it out to be. Negative values or 0 would still have been a problem though.

So we should really take this as an example of what to think about and what to test for when transofrming any data from the user.

I'll even reference this issue in the dev-notes guide, as an example of why unit testing us super important. We have a lot of tests for check_data, but I purposefully ignored checking ss_numeric as it seemed super straightforward. We should always ask "can this go wrong without producing errors", rather than "is this a complicated enough to be worth testing"

The most ironic part is that a couple of hours ago I used for the first time the test coverage tool described here (https://r-pkgs.org/testing-design.html#sec-testing-design-coverage). And I added tests for other things that were missing, but when I glanced over this line in red below, I thought "no point in testing that"! :)

[venpopov]

fixed by be789f6