philchalmers / mirt

Multidimensional item response theory
199 stars 75 forks source link

Error for multiple p-value adjust values #233

Closed drvictorvs closed 1 year ago

drvictorvs commented 1 year ago

Hey, all—but especially Phil Chalmers,

This looks like a quick fix. I just wanted to say that it was very annoying that after more than six hours running a DIF on a 10 million case database with the following code:

DIFLik <- mirt::DIF(MGF1,
                      which.par = c('a1', 'd1', 'd2', 'd3'),
                      scheme = 'drop',
                      items2test = NSE,
                      seq_stat = .05,
                      Wald = F,
                      p.adjust = c('bonferroni', 'holm', 'hochberg', 'hommel',
                                   'fdr', 'BY'),
                      verbose = T)

I got an error because of:

Error in if (p.adjust != "none") { : the condition has length > 1

That's just... 🤦🏻‍♂️.

philchalmers commented 1 year ago

That's expected behaviour as p.adjust() only accepts one input anyway.

set.seed(123)
x <- rnorm(50, mean = c(rep(0, 25), rep(3, 25)))
p <- 2*pnorm(sort(-abs(x)))

round(p, 3)
round(p.adjust(p), 3)
round(p.adjust(p, "BH"), 3)

# error
round(p.adjust(p, c('holm', 'BH')), 3)

But I'm happy to throw an error earlier to avoid learning about this.

drvictorvs commented 1 year ago

That's expected behaviour as p.adjust() only accepts one input anyway.

set.seed(123)
x <- rnorm(50, mean = c(rep(0, 25), rep(3, 25)))
p <- 2*pnorm(sort(-abs(x)))

round(p, 3)
round(p.adjust(p), 3)
round(p.adjust(p, "BH"), 3)

# error
round(p.adjust(p, c('holm', 'BH')), 3)

But I'm happy to throw an error earlier to avoid learning about this.

Yeah, that's what I meant. It's fine that it's expected behavior, it's just that it should probably come before the six hours of waiting 😂

philchalmers commented 1 year ago

True, but anticipating how all functions not written in the package error out is tricky, so these things can happen. Also, with 10 million cases I'd probably use the wald input as the quadratic approximation behaviour should give highly similar results to the LR method. Would certainly cut down on testing time.

drvictorvs commented 1 year ago

True, but anticipating how all functions not written in the package error out is tricky, so these things can happen. Also, with 10 million cases I'd probably use the wald input as the quadratic approximation behaviour should give highly similar results to the LR method. Would certainly cut down on testing time.

I know what you mean. I hope I didn't come off like I was making fun of your code or anything, I just found the whole thing funny. Of course these things happen. I'm very thankful to your and to whomever else contributed to this package, and thanks for the tip.

philchalmers commented 1 year ago

Ah I see, thanks for clarifying (I couldn't tell the tone of the post; no harm taken). If you see other instances of this in the package (which are likely there) feel free to open further issues or send pull requests. Cheers.