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 phyloseq_tax_glom #6891

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

Conversation

RZ9082
Copy link
Contributor

@RZ9082 RZ9082 commented Mar 28, 2025

FOR CONTRIBUTOR:

  • I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
  • License permits unrestricted use (educational + commercial)
  • This PR adds a new tool or tool collection
  • This PR updates an existing tool or tool collection
  • This PR does something else (explain below)

@paulzierep

Copy link
Contributor

@paulzierep paulzierep left a comment

Choose a reason for hiding this comment

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

thanks, minor updates


physeq_file <- args[1]
tax_rank <- args[2]
use_counts <- "--counts" %in% args
Copy link
Contributor

Choose a reason for hiding this comment

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

I find optparse cleaner:

but this is optional

tax_table_agg <- cbind("OTU ID" = rownames(tax_table_agg), tax_table_agg)
}

if (use_counts) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see a reason to ever exclude it, would not make this optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would make sense to exclude it when using a single column for the specified rank. Because when leaving only the column of the specified rank the NA values will be group together so it will be confusing.
Especially if you include the counts in the same table. The counts of all the NA values will be summed and you and won't represent the values of the OTU ID that is randomly selected. This will be misleading.

<param argument="--counts" truevalue="--counts" falsevalue="" type="boolean" label="Include OTU counts" optional="true" help="OTU abundances will be included in the output"/>
<param argument="--exclude_na_values" truevalue="--exclude_na_values" falsevalue="" type="boolean" label="Exclude unknown 'NA' values" optional="true" help="Unknown value 'NA' will be removed"/>
<conditional name="single_rank_cond">
<param name="exclude_otu_ids" type="select" label="Exclude OTU IDs from output" help="OTU IDs will be excluded from the output table">
Copy link
Contributor

Choose a reason for hiding this comment

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

why not like: argument="--exclude_na_values" truevalue="--exclude_na_values" falsevalue="" type="boolean" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To not get this linting warning: WARNING (ConditionalParamTypeBool): Conditional [single_rank_cond] first param of type="boolean" is discouraged, use a select


- *Taxonomic rank for agglomeration:* Specify the rank to merge OTUs at.
- *Include OTU counts:* If enabled, OTU abundances will be included in the output.
- *Exclude OTU IDs:* If enabled, OTU IDs will be excluded from the output table.
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add that a random OTU id form the ranks used for agg is taken

- *Include OTU counts:* If enabled, OTU abundances will be included in the output.
- *Exclude OTU IDs:* If enabled, OTU IDs will be excluded from the output table.
- *Keep only specified rank column:* If enabled, only the selected taxonomic rank will be kept.
- *Exclude NA values:* If enabled, unknown will be removed.
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add all symbols removed ?

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.

4 participants