qmethod() does not work in fork of 1.4.0

[aiorazabala]

Hey @maxheld83 , When using your fork of 1.4.0, qmethod() does not work, returning this error:

Error in loa.unrot %*% rot.mat : 
  requires numeric/complex matrix/vector arguments

[aiorazabala]

Using either lipset data and another set

[maxheld83]

yikes, that's weird.

Using the below reproducible example, I cannot reproduce your error on my current master branch:

data(lipset)
lipset$spearman <- qmethod(
  dataset = lipset$ldata,
  nfactors = 3,
  rotation = "varimax",
  forced = TRUE
  , cor.method = "spearman"
)

Is this when you're calling just qmethod, or q.mrot.do / q.mrot.choose– sounds like a rotation-related error.

Could you provide a reproducible example @aiorazabala ?

Will try and fix this (if I can figure out what's going on) next week.

[aiorazabala]

Calling just qmethod()

Try the example in the qmethod() documentation (i.e. with lipset dataset)

[maxheld83]

nope, sorry, that does not reproduce the error either:

data(lipset)
results <- qmethod(lipset[[1]], nfactors = 3, rotation = "varimax")
summary(results)

Here's a hunch: is it possible that you don't have the current version of psych (1.5.8)? William Revelle only introduced the rot.mat object in question recently (at my behest).

[aiorazabala]

Indeed psych v. 1.5.4 But why would that break the function qmethod()? qmethod versions 1.2x do not have this bug

[maxheld83]

it's because in my fork, qmethod() now always returns a rotation matrix, which is itself retrieved from psych, but psych introduced this results object only recently.

See #171

I think it's necessary for qmethod to always carry a rotation matrix to enable manual rotations in a reproducible way. Even if no manual rotation is intended, it's nice to carry the rotation matrix just to know precisely what happened to the unrotated principal components. William Revelle suggested that this is good practice for any factor analysis, which is why he agreed to add it to psych.

I wouldn't consider this a bug per se.

This kind of thing might again be avoided if we implemented a proper dependency management as per #74. If properly set up packrat would then just go out and fetch the requisite psych version (>=1.5.4 in this case), or warn the user in a meaningful way if that is not possible.

[maxheld83]

anyway, is your problem solved with psych 1.5.4 @aiorazabala ?

You can always unload my fork after the importing business and then go back to your fork if you wish ...

[aiorazabala]

Yes, that's what I'm doing.

My concern is over changes in qmethod() function in general. I don't want it to be much modified by others, since it's been thoroughly tested and I can't give any guarantee of whether it's still OK after it undergoes changes I haven't made.

Lets discuss this in the next meeting. An alternative may be to provide the rotation matrix when running qmethod(), only as an additional argument set to FALSE by default. Or to modulate it in a separate function for whoever is interested, e.g. q.rotation.matrix(). Lets keep qmethod() as much usable and simple as possible---no unnecessary complications.

[maxheld83]

Mmh your call; I can make it optional defaulting to FALSe, if you insist.

Spinning it out in a separate function is problematic, because the rotation matrix should be stored upon rotation, which happens in qmethod. There's no robust/easy way to compute the rotation matrix after the fact I think.

I understand your concerns about not changing base functions, though I had hoped that the automatic testing I implemented would alleviate these concerns: qmethod does after all, still produce the exact same results.

I'm also worries that not adding to qmethod results will lead to a lot of complexity and special cases elsewhere in the package. For example it's somewhat difficult if sometimes, the results object has a rotation matrix, and sometimes not.

As a result I fear this could stultify future additions to the program.

Anyway, let's talk next week!

[maxheld83]

@aiorazabala just suggested an if clause to test which version of psych is available, run that code only if psych is current enough, otherwise return empty rot.mat

[aiorazabala]

packageVersion("psych")

[maxheld83]

I think this done?