docs: mention that you can merge multiple GMT files for fgsea #159
Conversation
…ultiple files to be specified
WalkthroughExpanded comments were added above the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
workflow/schemas/config.schema.yaml (1)
150-151: Add constraints to the gene_set_files object schema.The schema change from
gene_sets_file(string) togene_set_files(object) is correct, but the object type lacks validation constraints. Consider adding pattern properties or additional constraints to ensure the object values are valid file paths.gene_set_files: type: object + patternProperties: + "^.+$": + type: string + additionalProperties: falseworkflow/rules/enrichment.smk (2)
34-85: Consider statistical implications of separate fgsea runs.Running fgsea separately on multiple gene set files means each file gets independent multiple testing correction. This may not provide optimal false discovery rate control if you want to control FDR across all gene sets globally.
Consider documenting this behavior and potentially offering guidance on when users should merge GMT files versus using separate analyses.
38-40: Add wildcard constraints for robustness.Consider adding wildcard constraints to ensure
gene_set_filevalues are valid identifiers (e.g., alphanumeric with underscores/hyphens).wildcard_constraints: gene_set_file=r"[A-Za-z0-9_-]+"This prevents issues with special characters in file paths and improves error messages for invalid configurations.
Also applies to: 92-94
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.test/config/config.yaml(1 hunks).test/three_prime/config/config.yaml(1 hunks)config/config.yaml(1 hunks)workflow/rules/common.smk(1 hunks)workflow/rules/enrichment.smk(4 hunks)workflow/schemas/config.schema.yaml(1 hunks)
🔇 Additional comments (7)
.test/config/config.yaml (1)
94-95: LGTM! Test configuration properly reflects the new structure.The test configuration correctly implements the new
gene_set_filesdictionary structure, maintaining consistency with the main configuration format.workflow/schemas/config.schema.yaml (1)
157-157: LGTM! Required fields updated correctly.The required fields list has been properly updated to reflect the schema change from
gene_sets_filetogene_set_files..test/three_prime/config/config.yaml (1)
94-95: LGTM! Consistent implementation with appropriate test naming.The three_prime test configuration correctly implements the new structure using "test" as the key, demonstrating the flexibility of the named gene set files approach.
workflow/rules/enrichment.smk (4)
38-40: LGTM! Clean implementation of dynamic gene set file lookup.The lookup function correctly retrieves gene set file paths from the new nested config structure. This enables flexible specification of multiple gene set files.
44-44: Consistent wildcard propagation across all fgsea outputs and logs.All output files and logs now properly include the
{gene_set_file}wildcard, ensuring results are organized by gene set file. The naming convention is consistent and logical.Also applies to: 50-50, 56-56, 62-62, 68-68, 82-82
92-95: Proper dependency maintenance between fgsea rules.The
fgsea_plot_gene_setsrule correctly:
- Uses the same lookup pattern for gene set files
- References the parameterized output from the
fgsearule- Maintains the dependency chain with consistent wildcard usage
99-99: Consistent output directory and log file parameterization.The output directory and log file correctly include the
{gene_set_file}wildcard, maintaining consistency with the overall design.Also applies to: 111-111
|
Feedback from @johanneskoester was also, that users should rather combine their gene sets into a single GMT file for a joint gene set enrichment analysis. So I'll update this pull request to simply improve the docs. |
dlaehnemann
changed the title
johanneskoester
deleted the
feat/allow-for-multiple-gene-set-files-for-fgsea
branch
This change allows for multiple gene set files to be specified.
There are two things to especially consider in reviewing this:
.gmtfiles manually, to have just one comprehensive run of fgsea? This might be the favorable, as this would ensure multiple testing control across all specified gene sets at once. If we decide to do this, I would create a separate pull request to properly explain this in theconfig.yaml(and possiblyeREADME.md) files.3.0.0release with a minor version bump?Summary by CodeRabbit