Skip to content

12801 bug in gsea module related with multiple gene sets processing #12821

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.

Sign up for GitHub

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

Open
brovolia wants to merge 2 commits into
base: master
Choose a base branch
from
+12 −6
Open

12801 bug in gsea module related with multiple gene sets processing #12821

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
38e4e31
using quoting for file names if possible
brovolia Aug 26, 2026
8c1d769
dropping comments
brovolia Aug 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions modules/nf-core/gsea/gsea/main.nf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,26 +45,32 @@ process GSEA_GSEA {
def prefix = task.ext.prefix ?: "${meta.id}"
def rpt_label = prefix.replaceAll('\\.$', '') // Remove any trailing dots from prefix when passed as report label, so GSEA doesn't produce double-dotted top-level outputs
def chip_command = chip ? "-chip $chip -collapse true" : ''

@pinin4fjords pinin4fjords Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since everything else is getting quoted:

Suggested change
def chip_command = chip ? "-chip $chip -collapse true" : ''
def chip_command = chip ? "-chip \"$chip\" -collapse true" : ''

// gsea-cli crashes if given more than one -gmx file; callers must invoke this process once per gene set file.

@pinin4fjords pinin4fjords Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should probably quote the file in the chip_command too

@pinin4fjords pinin4fjords Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This comment's wrong once gmx is comma-joined — worth dropping.

def gmx = gene_sets instanceof List ? gene_sets[0] : gene_sets

@pinin4fjords pinin4fjords Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think silently selecting the first gene set is the right workaround, I think that's worse than erroring when someone supplied the wrong input shape.

We should either explicitly error when a list is supplied, or just remove this and let the fail continue (since any error is at the workflow level)

@brovolia brovolia Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree, I will drop the instanceof List entirely

@pinin4fjords pinin4fjords Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually, I think maybe we just need to make a comma-separated version?

Suggested change
def gmx = gene_sets instanceof List ? gene_sets[0] : gene_sets
def gmx = gene_sets instanceof List ? gene_sets.join(',') : gene_sets

@brovolia brovolia Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, in this case, gsea-cli will generate an analysis name by appending the list of collection names. I think it should be fixed at the pipeline-level

@pinin4fjords pinin4fjords Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

gsea-cli doesn't crash on multiple files — it silently keeps only the first and drops the rest (confirmed with a real run). It does support multiple files via one comma-joined value (confirmed against GeneSetMatrixMultiChooserParam and a real two-file run combining both, Got gsets: 50).

Suggested change
def gmx = gene_sets instanceof List ? gene_sets[0] : gene_sets
def gmx = gene_sets instanceof List ? gene_sets.join(',') : gene_sets

@brovolia brovolia Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, so here is the question then, do you want one GSEA_GSEA call per contrast with multiple slurm jobs or a multi-collection call per contrast so gsea-cli will handle it? If you want the second option, then I guess conf/modules.config should also be corrected after

@pinin4fjords pinin4fjords Aug 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure there's a good reason to separate them into multiple jobs?

But in any case, it's a slightly orthogonal question. From the module's point of view as a component it should be able to deal with the multiple-input method, since the GSEA CLI can.


"""
# Run GSEA

gsea-cli GSEA \\
-res $gct \\
-cls ${cls}#${target}_versus_${reference} \\
-gmx $gene_sets \\
-res "$gct" \\
-cls "${cls}#${target}_versus_${reference}" \\
-gmx "$gmx" \\
$chip_command \\
-out . \\
--rpt_label $rpt_label \\
--rpt_label "$rpt_label" \\
$args

# Un-timestamp the outputs for path consistency
mv ${rpt_label}.Gsea.*/* .
# Only rmdir actual directories: the .rpt file's name also matches "*.Gsea.*".
mv *.Gsea.*/* .

@pinin4fjords pinin4fjords Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it necessary to have made this glob broader?

@brovolia brovolia Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah.. I will revert it back

for gsea_dir in *.Gsea.*/; do
[ -d "\$gsea_dir" ] && rmdir "\$gsea_dir"
done
Comment on lines +64 to +68

@pinin4fjords pinin4fjords Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tried to reproduce this naming problem — a real comma-joined -gmx across two GMT files produced a plain test_comma3.Gsea.<timestamp> dir, which the original glob already matches. Checked ToolReport.generateReportDir in gsea-desktop v4.3.2 too — built from --rpt_label + tool name + timestamp only. Suggest reverting once gmx is comma-joined:

Suggested change
# Only rmdir actual directories: the .rpt file's name also matches "*.Gsea.*".
mv *.Gsea.*/* .
for gsea_dir in *.Gsea.*/; do
[ -d "\$gsea_dir" ] && rmdir "\$gsea_dir"
done
mv ${rpt_label}.Gsea.*/* .

timestamp=\$(cat *.rpt | grep producer_timestamp | awk '{print \$2}')

for pattern in _\${timestamp} .\${timestamp}; do
find . -name "*\${pattern}*" | sed "s|^\\./||" | while read -r f; do
mv \$f \${f//\$pattern/}
mv "\$f" "\${f//\$pattern/}"
done
done
sed -i.bak "s/[_\\.]\$timestamp//g" *.rpt *.html && rm *.bak
Expand Down
Loading