-
Notifications
You must be signed in to change notification settings - Fork 27
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
LDA ordination with feature loadings computation #598
Conversation
Great! I will be first looking fwd to comments from @TuomasBorman and then let's see if I have anything to add. |
Conflicts to resolve? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Even though the code is rather short, structure it by adding tests and comments
…ome/mia into feature_loadings_methods
Couple things to discuss:
|
Intuitively, reducedDim is a logical place given that this would be usually done for samples. But it could be in principle applied on both rows and columns. The function generates both feature matrix and feature loading matrix, both should be stored. LDA is a matrix decomposition method, and I would not recommend to link it with clustering so directly. But it could in principle go to the bluster package. Not sure if they would approve but it could be asked by opening an issue. That might take more time, however.. therefore I would meanwhile proceed with implementing these in mia, and later we can move them elsewhere if necessary? LDA and NMF have clear uses, it would be useful to have them easily accessible. |
With these explanations, I am fine with mia implementation and reducedDim() |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #598 +/- ##
========================================
Coverage ? 68.08%
========================================
Files ? 44
Lines ? 5273
Branches ? 0
========================================
Hits ? 3590
Misses ? 1683
Partials ? 0 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor things
Ok to me to add that. However, if it feels a lot to add now, I would focus on finalizing the present one (with a single fixed k) first, and then adding that multi-k optimization as a separate PR. |
That would require something like this
|
Merge branch 'devel' of https://github.com/microbiome/mia into feature_loadings_methods # Conflicts: # NEWS
NMF would go with this same template I think. However, there k (rank) can be numeric vector and the function finds optimal k by itself |
For LDA we could have another PR for multiple k values, if you are not able to implement it now The idea was this:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this PR ready? If the modification on multiple k values do not come to this PR, I think this is good to go
This PR adds two new methods
getLDA
andaddLDA
that perform Latent Dirichlet Allocation.getLDA
returns the ordination matrix with the feature loadings matrix stored as attribute.addLDA
adds this ordination matrix toreducedDims
of the TreeSummarizedExperiment.This PR is related to issue #596.