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

add support for groups other than rank in plotAbundance #148

Merged
merged 7 commits into from
Oct 1, 2024
Merged

Conversation

Daenarys8
Copy link
Contributor

This commit deprecates rank and adds support for other columns in rowData
ping: #100

@TuomasBorman TuomasBorman changed the title add support for groups other than rank in plotAbundancee add support for groups other than rank in plotAbundance Sep 1, 2024
R/plotAbundance.R Outdated Show resolved Hide resolved
@TuomasBorman
Copy link
Contributor

Thanks, seems to work! However, I think we should think if this is needed as this makes the function more complex. The same can be achieved as follows which might be more transparent (I prefer that)

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

rowData(tse)$bacteria_group <- sample(c("gram-pos", "gram-neg"), nrow(tse), replace = TRUE)

# Works fine
plotAbundance(tse, group = "bacteria_group", as.relative = TRUE)

# The alternative apporach
tse_sub <- agglomerateByVariable(tse, f = "bacteria_group", by = "rows")
tse_sub <- transformAssay(tse_sub, method = "relabundance")
plotAbundance(tse_sub, assay.type = "relabundance")

Second thing is that, group argument name seems not to be used in anywhere else. It might be good to have same naming as in other functions because that might help users to understand what the arguments do.

Instead of changing the name, one option is to "hiddenly" support other grouping variables. The name could be rank, but it would still work with other variables also. As grouping by rank is the most common thing, that might make the function more simple to user

@TuomasBorman
Copy link
Contributor

This naming is related to issue/PR that is open in mia. Wait until we have fixed that.

@TuomasBorman
Copy link
Contributor

microbiome/mia#623

Signed-off-by: Daena Rys <[email protected]>
R/plotAbundance.R Outdated Show resolved Hide resolved
R/plotAbundance.R Outdated Show resolved Hide resolved
@TuomasBorman
Copy link
Contributor

We decided to prefer that user does (relative) transformation outside the function. That is why as.relative was moved to "hidden" parameter. This means that also agglomeration should be done outside the function. --> I moved rank/group parameter as hidden parameter.

@TuomasBorman TuomasBorman merged commit 20beb75 into devel Oct 1, 2024
3 checks passed
@TuomasBorman TuomasBorman deleted the group branch October 1, 2024 14:53
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.

2 participants