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

Adds option to perform EFA with ordinal variables and two other options (Mardia's tests; Parallel Analysis detailed output) #75

Merged
merged 34 commits into from
Jul 15, 2022

Conversation

francotisocco
Copy link
Contributor

@francotisocco francotisocco commented Dec 30, 2021

.....

Hi! I modified the jaspFactor module to fit my analysis needs and then thought it could also be of use to others. I am not heavily experienced with Github so I am not sure whether I am conducting the pull request correctly - if anything, let me know!

Moreover, apologies in advance for posting so close to New Years.

@francotisocco francotisocco changed the title Adds option to perform EFA ordinal variables and a couple other options (Mardia's tests; Parallel Analysis detailed output) Adds option to perform EFA with ordinal variables and two other options (Mardia's tests; Parallel Analysis detailed output) Dec 30, 2021
@TimKDJ TimKDJ requested a review from vandenman January 4, 2022 10:28
@juliuspfadt
Copy link
Collaborator

Hi @francotisocco thank you very much for the PR and providing the fruits of your work for others. I apologise for the delay in looking into your PR :)
I started going over this and I am having trouble getting the poly matrix to run with a simple data set.
example_asrm.jasp.zip
It seems the same data set also does not work in R with the psych package. For example psych::polychoric gives the error JASP also displays. Do you have any idea why that happens?

Copy link
Collaborator

@juliuspfadt juliuspfadt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again. This already looks really good. Some minor changes are in order. Also, do you feel comfortable making some unit-tests for the added features? Otherwise I can also do that.

@francotisocco
Copy link
Contributor Author

francotisocco commented Feb 17, 2022

Hi @francotisocco thank you very much for the PR and providing the fruits of your work for others. I apologise for the delay in looking into your PR :) I started going over this and I am having trouble getting the poly matrix to run with a simple data set. example_asrm.jasp.zip It seems the same data set also does not work in R with the psych package. For example psych::polychoric gives the error JASP also displays. Do you have any idea why that happens?

Hi Julius! Don't apologize at all, since I know the PR introduces many little things and nuances - It is probably me who should apologize :). Thank you deeply for your extensive reviews - I'll be responding to each suggestion within the next days.

Regarding what you mentioned about the dataset, I'll look into it around these days. It could be convergence issues (because it may happen with trying to estimate the poly matrix with few observations), although I wouldn't want to guess without taking a deeper look.

EDIT - UPDATE: I've been looking into it and it seems that the [example_asrm.jasp.zip] dataset has a combination that psych "does not like" while computing the poly mat - either via psych::polychoric or psych::mixedCor: a relatively "small" sample size + some response alternatives not present in certain variables while present in others (i.e., zero-cell entries when computing the matrix).

When it tries to correct for the presence of zero-cell entries in the presence of a "small sample size", the function apparently fails for "yet undetermined reasons" (this last phrase is a verbatim quote from the Psych documentation).

Further inspecting, it appears that the variable asrm_3 is missing the response category "5". If you remove it from the EFA query, the analysis runs properly - of course this is not 100% desirable since it's perfectly possible to have a dataset with missing response categories within variables "in real life".

Facing this, I could:
a) Include a clarification within the help file similar to what I wrote above.

b) Work on including a correction within the rscript so that if the function resolves in an error, the computation runs again without attempting to correct for continuity.

@juliuspfadt
Copy link
Collaborator

juliuspfadt commented Feb 18, 2022

I think it would be fine to just display a more informative error message, no need to fix the function from psych. This message could just read: "Unfortunately, the estimation of the polychoric/tetrachoric correlation matrix failed. This might be due to a small sample size or variables not containing all response categories."
Obviously, adding some info to the help file is also good.

@juliuspfadt
Copy link
Collaborator

fixes jasp-stats/jasp-issues#1624

@juliuspfadt
Copy link
Collaborator

Hi @francotisocco it has been a while, and I just wanted to check in if there is anything I can still help you with for this PR?

@francotisocco
Copy link
Contributor Author

Hi @JuliusPf! Sorry for the delay - i've had a surge of work and I'll probably be able to get back to these changes around April. I apologise! Regarding the unit-tests, I'm afraid I wouldn't know how to proceed to perform them. Of course, feel free to implement changes if you find a suitable moment; otherwise in a few weeks I'll get hands-on. Again, deep apologies for the delay and thank you for your work and follow-up!

PS: excellent you could also implement this within PCA :)

@juliuspfadt
Copy link
Collaborator

No need to apologise, you are doing us a favour! I can add the unit tests once you are done with the PR, no worries.

Copy link
Contributor Author

@francotisocco francotisocco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay! I believe I've addressed all comments/reviews - by the way, thank you again for providing them since it helps me understand more about how the program works conjointly with R. If there were any further necessary modifications, don't hesitate in pointing them out.

francotisocco and others added 17 commits July 5, 2022 15:36
Co-authored-by: Julius Pfadt <38500953+juliuspf@users.noreply.github.com>
Co-authored-by: Julius Pfadt <38500953+juliuspf@users.noreply.github.com>
Co-authored-by: Julius Pfadt <38500953+juliuspf@users.noreply.github.com>
Co-authored-by: Julius Pfadt <38500953+juliuspf@users.noreply.github.com>
Co-authored-by: Julius Pfadt <38500953+juliuspf@users.noreply.github.com>
Co-authored-by: Julius Pfadt <38500953+juliuspf@users.noreply.github.com>
Co-authored-by: Julius Pfadt <38500953+juliuspf@users.noreply.github.com>
Co-authored-by: Julius Pfadt <38500953+juliuspf@users.noreply.github.com>
Co-authored-by: Julius Pfadt <38500953+juliuspf@users.noreply.github.com>
Co-authored-by: Julius Pfadt <38500953+juliuspf@users.noreply.github.com>
Co-authored-by: Julius Pfadt <38500953+juliuspf@users.noreply.github.com>
Co-authored-by: Julius Pfadt <38500953+juliuspf@users.noreply.github.com>
@juliuspfadt juliuspfadt force-pushed the master branch 3 times, most recently from 847a3a5 to d1c4cab Compare July 5, 2022 15:36
@vandenman vandenman merged commit 17fbf2d into jasp-stats:master Jul 15, 2022
@vandenman
Copy link
Contributor

@francotisocco thanks for contributing!

@francotisocco
Copy link
Contributor Author

@francotisocco thanks for contributing!

On the contrary! Thank you for working with what I could put together. A pleasure.

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