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

Isn't the 3pl model wrong? #38

Closed
muriloime opened this issue Sep 15, 2022 · 6 comments
Closed

Isn't the 3pl model wrong? #38

muriloime opened this issue Sep 15, 2022 · 6 comments

Comments

@muriloime
Copy link

muriloime commented Sep 15, 2022

Shouldn't it be dist.Bernoulli(probs=lambdas[items]+(1-lambdas[items]) * p_star), uinstead of dist.Bernoulli(probs=(1-lambdas[items]) * p_star),

@EntilZha
Copy link
Collaborator

Hi, thanks for the bug report! Without digging super deep, I think this is probably a notional/naming issue. I believe that this and the 4pl models were developed similarly and mainly for the purpose of adding on an additional parameter to the 2PL representing whether an example was doable at all. I.E., if a particular item is non-answerable, then the parameter value should be zero or one. This is different from 3PL models in the literature, hence the unfortunate naming problem of having 3 parameters despite not being the traditional model (in our case, we expected random guess to be near zero).

So given that, if lambda were to represent the guessing parameter, you are correct, it should be changed. In our original implementation though, lambda was meant to be correlated with feasibility. Even given that, I looked at our 3PL/4PL code and for some weird reason we seem to have (1-lambda) in one place, but (lambda) in another, and they really should be consistent.

I'm guessing that the best long term solution is to come up with some naming or constructor that can make this a bit more explicit, i.e., give me a model which has feasibility, but not discriminability. In any case, we really should have a standard 3PL model where the third parameter is a guessing parameter. We'd welcome a PR for it of course too! Thanks and hopefully that answers your question.

@muriloime
Copy link
Author

Thanks for the reply @EntilZha .

Yes, I had saw that too, it is inconsistent with predict function. I also have observed a couple of unexpected things, such as the choice of priors giving wrong results, and redundant parameters in the multidimensional ( I expected to have a*theta - d formulation instead of a*(theta -b) one). More than happy to discuss/help with these points

cheers
Murilo

@muriloime
Copy link
Author

(if you can help with a naming convention I can definitely open a PR with this issue in particular)

@jplalor
Copy link
Collaborator

jplalor commented Sep 19, 2022

Thanks, Murilo!

To stay consistent with the traditional 3PL model, I think we should make the change that you mentioned initially (If you can open a PR for that, that would be great!).

For feasibility, maybe we should rename 4PL to something that more explicitly references feasibility.

@EntilZha I agree about the long-term fix, something that is more explicit is probably ideal.

@jplalor
Copy link
Collaborator

jplalor commented Sep 26, 2022

3PL fix is done, so I'll close this issue. We can address the other items via new issues. Thanks @muriloime!

@jplalor jplalor closed this as completed Sep 26, 2022
@charchit7
Copy link
Contributor

Thanks, @muriloime @jplalor @EntilZha

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

No branches or pull requests

4 participants