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

plotAbundanceDensity: added boxplot and violin #143

Merged
merged 8 commits into from
Aug 12, 2024
Merged

plotAbundanceDensity: added boxplot and violin #143

merged 8 commits into from
Aug 12, 2024

Conversation

Daenarys8
Copy link
Contributor

ping: #142

@Daenarys8 Daenarys8 requested a review from TuomasBorman July 24, 2024 13:58
@antagomir
Copy link
Member

DId you add (at least a short) description of the plotting options in the roxygen documentation? I did not find this.

@Daenarys8
Copy link
Contributor Author

DId you add (at least a short) description of the plotting options in the roxygen documentation? I did not find this.

Thanks. I have added to the options.

@antagomir
Copy link
Member

It is good to update documentation by default always.

@antagomir
Copy link
Member

Some checks fail?

@Daenarys8
Copy link
Contributor Author

Some checks fail?

The checks are unrelated to this PR. I could prepare another PR with a fix.

@Daenarys8
Copy link
Contributor Author

I think, this is good to go.

Copy link
Contributor

@TuomasBorman TuomasBorman left a comment

Choose a reason for hiding this comment

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

Let's rethink this. It seems that the desired plot types are already available in SE ecosystem. We should avoid overlapping work. Check it these plots fulfill the need

library(miaViz)

data("peerj13075", package = "mia")
tse <- peerj13075

tse <- transformAssay(tse, method = "relabundance")

plotAbundanceDensity(tse, assay.type = "relabundance", n = 5, layout = "violin")
plotAbundanceDensity(tse, assay.type = "relabundance", n = 5, layout = "box")

library(scater)

top <- getTop(tse)

plotExpression(tse, features = top, assay.type = "relabundance") + ggplot2::coord_flip()
plotExpression(tse, features = top, assay.type = "relabundance", show_violin = FALSE, show_box = TRUE) + ggplot2::coord_flip()


@Daenarys8
Copy link
Contributor Author

plotExpression(tse, features = top, assay.type = "relabundance", show_violin = FALSE, show_box = TRUE) + ggplot2::coord_flip()

Thanks.

@Daenarys8
Copy link
Contributor Author

Closing issue now.

@Daenarys8 Daenarys8 closed this Aug 9, 2024
@antagomir
Copy link
Member

Can you summarize what was done regarding the point on plotExpression vs. plotAbundanceDensity, @Daenarys8 ?

@Daenarys8
Copy link
Contributor Author

Can you summarize what was done regarding the point on plotExpression vs. plotAbundanceDensity, @Daenarys8 ?

plotExpression provides the desired layouts violin and boxplot which can be used to measure counts based on selected taxa.

@antagomir
Copy link
Member

Can you summarize briefly how this was implemented? Does plotAbundanceDensity internally call plotExpression?

@Daenarys8 Daenarys8 reopened this Aug 9, 2024
@Daenarys8
Copy link
Contributor Author

Can you summarize briefly how this was implemented? Does plotAbundanceDensity internally call plotExpression?

I had a misunderstanding of the intention. I have added the changes.

@TuomasBorman
Copy link
Contributor

Can you summarize briefly how this was implemented? Does plotAbundanceDensity internally call plotExpression?

Does it have to call it internally? It causes more maintaining work comparing to if we just rely on plotExpression?

Signed-off-by: Daena Rys <[email protected]>
@antagomir
Copy link
Member

Can you summarize briefly how this was implemented? Does plotAbundanceDensity internally call plotExpression?

Does it have to call it internally? It causes more maintaining work comparing to if we just rely on plotExpression?

Yes it might be so.

I did not mean to suggest how it should be done, just asked for a summary on what was done as I could not immediately figure it out from the sources.

Good to discuss first how to proceed, then proceed.

If plotExpression already solves the issue, then the following actions could be useful:

  • mention this in the @Seealso field of plotAbundanceDensity and perhaps even in its @examples field (for comparison)
  • have examples of both in OMA in the same place so that it is easy to find; not all variants have to be shown in OMA, just those we like to recommend by default (+ mention about the other available options)

Did I miss something?

@Daenarys8
Copy link
Contributor Author

I have mentioned in @Seealso and @examples.
Now for OMA, I could add to chapter 10.1, It is the most suitable place since it mentions abundance plots and jitter and density plots are available there.

Signed-off-by: Daena Rys <[email protected]>
@Daenarys8 Daenarys8 requested a review from TuomasBorman August 12, 2024 12:10
@TuomasBorman
Copy link
Contributor

Good place for plotExpression is also differential abundance chapter as it does not have abundance visualization yet. It is very common to visualize the significant taxa. https://microbiome.github.io/OMA/docs/devel/pages/differential_abundance.html

There could be a link in QC chapter. "Abundances can be visualized also with plotExpression(). See example from here"

@TuomasBorman TuomasBorman merged commit 102c876 into devel Aug 12, 2024
3 checks passed
@TuomasBorman TuomasBorman deleted the layout branch August 12, 2024 12:17
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