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

scaled_fa update: s beomces zero #12

Open
yunqiyang0215 opened this issue Dec 7, 2021 · 12 comments
Open

scaled_fa update: s beomces zero #12

yunqiyang0215 opened this issue Dec 7, 2021 · 12 comments

Comments

@yunqiyang0215
Copy link
Collaborator

I noticed that from the ukbblood cell data example, an error arises possibly due to the fact the updated s becomes 0 in the current iteration. Then in the next iteration, estimated s will be NA. In the previous iteration, s=0.002099687.

The bug should occur in fa_scaled_iid() and fa_scaled_notiid(). The input s shouldn't be 0. One way is to add a constraint to s before running fa_scaled_iid().

However, if input s=0, fa_scaled_iid() couldn't run. Not sure how s=NA is created.

@pcarbo
Copy link
Member

pcarbo commented Dec 8, 2021

Well if s is being set to zero then this means that the likelihood is being maximized at s = 0? Or is this a bug? I would verify this first. If it is correct—not a bug—then this means that this mixture component is behaving as a delta mass. I'm not sure how best to deal with this case, but it seems that the simplest approach would be to constrain s to be positive. We could potentially accommodate s = 0 but it would make the code significantly more complicated.

@stephens999
Copy link
Collaborator

stephens999 commented Dec 8, 2021 via email

@stephens999
Copy link
Collaborator

stephens999 commented Dec 8, 2021 via email

@stephens999
Copy link
Collaborator

stephens999 commented Dec 8, 2021 via email

@pcarbo
Copy link
Member

pcarbo commented Dec 8, 2021

Okay, then in some parts of the code an if-else statement will be needed check for the special case that the prior covariance matrix U is a matrix of zeros.

@yunqiyang0215
Copy link
Collaborator Author

I added a check to fa_scaled_iid() and fa_scaled_notiid() to check whether s==0 or U0 == a matrix of all 0s. And I checked on simple simulated data by input a matrix of all 0s, it works fine.

@pcarbo
Copy link
Member

pcarbo commented Dec 8, 2021

What happens to the posterior probabilities computed in compute_posterior_probs for U matrices that are all zeros? Are the posterior probabilities also zero?

@yunqiyang0215
Copy link
Collaborator Author

In the real data example, s = 0 component has exactly 0 weight. While for the matrix of all 0s, the weight is very close to 0, 3.250872e-317. But I don't think there is a relationship between s=0/U all zeros and weight being 0?

@stephens999
Copy link
Collaborator

stephens999 commented Dec 9, 2021 via email

@pcarbo
Copy link
Member

pcarbo commented Dec 9, 2021

Sorry that's an error on my part.

It does make me think that for some of the U updates we might want to add checks for U being a matrix of zeros. For example, if U is a matrix of zeros, will the ED update have any effect?

@stephens999
Copy link
Collaborator

stephens999 commented Dec 9, 2021 via email

@pcarbo
Copy link
Member

pcarbo commented Dec 9, 2021

Yeah so we might want to give a warning or error for this case since it could conceivably happen.

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

3 participants