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

The first element of assays in SE might be a matrix when using DESeq2 method #269

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xiangpin
Copy link

The related issue

> library(tidybulk)
========================================
tidybulk version 1.12.0
If you use TIDYBULK in published research, please cite:

Mangiola et al. tidybulk: an R tidy framework for modular
transcriptomic data analysis. Genome Biology 2021.

This message can be suppressed by:
  suppressPackageStartupMessages(library(tidybulk))
========================================
> library(SummarizedExperiment) %>% suppressPackageStartupMessages()
> assays(se_mini)[[1]] <- assays(se_mini)[[1]] %>% data.frame()
> se_mini %>% identify_abundant() %>% test_differential_abundance(.formula=~condition, method='DESeq2')
No group or design set. Assuming all samples belong to one group.
renaming the first element in assays to 'counts'
Error in DESeq2::DESeqDataSet(., design = .formula) :
  counts matrix should be numeric, currently it has mode: list
>

when convert it to matrix, it works

> assays(se_mini)[[1]] <- assays(se_mini)[[1]] %>% as.matrix()
> se_mini %>% identify_abundant() %>% test_differential_abundance(.formula=~condition, method='DESeq2')
No group or design set. Assuming all samples belong to one group.
renaming the first element in assays to 'counts'
converting counts to integer mode
estimating size factors
estimating dispersions
gene-wise dispersion estimates
mean-dispersion relationship
-- note: fitType='parametric', but the dispersion trend was not well captured by the
   function: y = a/x + b, and a local regression fit was automatically substituted.
   specify fitType='local' or 'mean' to avoid this message next time.
final dispersion estimates
fitting model and testing
tidybulk says: to access the raw results (fitted GLM) do `attr(..., "internals")$DESeq2`
class: SummarizedExperiment
dim: 527 5
metadata(0):
assays(1): count
rownames(527): ABCB4 ABCB9 ... ZNF324 ZNF442
rowData names(8): entrez .abundant ... pvalue padj
colnames(5): SRR1740034 SRR1740035 SRR1740043 SRR1740058 SRR1740067
colData names(5): Cell.type time condition days dead
>

@stemangiola
Copy link
Owner

stemangiola commented May 19, 2023

@mikelove in case you have any opinion

@mikelove
Copy link
Contributor

Great!

@stemangiola
Copy link
Owner

stemangiola commented May 20, 2023

Thanks for the PR @xiangpin !

Maybe I'm missing the obvious but why should we add a assay that is a dataframe where DESeq2 needs counts. I fail to see the use-case could you be more explicit?

@mikelove
Copy link
Contributor

I would say that having a data.frame instead of matrix in the SummarizedExperiment is not typical.

@xiangpin
Copy link
Author

Yes, a data.frame in the SummarizedExperiment is not typical. But for many users, they might also import a data frame to assays when using the SummarizedExperiment function to build a SummarizedExperiment object, since the help information of SummarizedExperiment show that assays: A ‘list’ or ‘SimpleList’ of matrix-like elements, or a matrix-like object (e.g. an ordinary matrix, a data frame, a DataFrame object from the ‘S4Vectors’ package, a sparseMatrix derivative from the ‘Matrix’ package, a DelayedMatrix object from the ‘DelayedArray’ package, etc...). and data frame might be more similar to other objects for many users.

@stemangiola
Copy link
Owner

stemangiola commented May 31, 2023

As the original SE is not changed (but only just the DESeq2 object, passed to the internals for reproducibility, is), I think this PR is great.

@xiangpin The only thing I ask before pushing is a unit test that would fail before PR (current master) and produce the expected result with the PR.

Thanks for this PR!

@stemangiola stemangiola self-requested a review June 21, 2023 03:31
Copy link
Owner

@stemangiola stemangiola left a comment

Choose a reason for hiding this comment

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

Hello @xiangpin, thanks for the pull request.

Please add a unit test that would fail before PR (current master) and produce the expected result with the PR.

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