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

update fit_cov_ebnmf #4

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

willwerscheid
Copy link
Member

use flash_update_data and lowrank plus sparse representation

@pcarbo
Copy link
Member

pcarbo commented Nov 8, 2023

@willwerscheid Is the idea here that this new flash_fit_cov_ebnmf should produce the same result as the old flash_fit_cov_ebnmf, but the new version should be much faster than the old version when Y is sparse?

@willwerscheid
Copy link
Member Author

Yes, this should produce the same result. (If I add flash_factors_split then the result could be very slightly different since I would keep the values of EL2 and EF2 when doing the splitting.) I am not sure about it being much faster. Hopefully at least somewhat faster but I don't have any guesses about how much. We will need to do some testing. I think @YushaLiu was planning to test on the pdac dataset.

@YushaLiu
Copy link
Collaborator

YushaLiu commented Dec 4, 2023

Hi @willwerscheid, sorry I wanted to try this earlier on the pancreatic cancer data but my RCC account was locked and just got reactivated. I went over this updated file, and have a few quick questions.

In line 134 and 137, you use * for matrix multiplication. Do you want to do element wise matrix multiplication here? In particular, in line 137, it seems L.pm and F.pm do not have the same dimension. Does it make sense to use * to multiply them?
With your new definition of fit_ebmf_to_YY, my understanding is that dat can be a covariance matrix or a low rank representation depending on the size of Y. If that is the case, in line 142-143 I think you also need to consider separately when dat is a matrix or a list?

@willwerscheid
Copy link
Member Author

I think lines 134 and 137 are correct. You should not need to reconstruct the full matrix to get the diagonals. L.pm and F.pm are the same dimension; n = p because it is a covariance matrix. But I think you are correct about lines 142-3. There should be a line handling the case where dat is a matrix.

@YushaLiu
Copy link
Collaborator

YushaLiu commented Dec 5, 2023

I think lines 134 and 137 are correct. You should not need to reconstruct the full matrix to get the diagonals. L.pm and F.pm are the same dimension; n = p because it is a covariance matrix. But I think you are correct about lines 142-3. There should be a line handling the case where dat is a matrix.

Oh you are right! I forgot that here L.pm and F.pm are the same dimension because n=p here.

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

Successfully merging this pull request may close these issues.

3 participants