Skip to content

Conversation

antoniasaracco
Copy link
Contributor

PR checklist

Closes #XXX

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Emit the versions.yml file.
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • For modules:
      • nf-core modules test <MODULE> --profile docker
      • nf-core modules test <MODULE> --profile singularity
      • nf-core modules test <MODULE> --profile conda
    • For subworkflows:
      • nf-core subworkflows test <SUBWORKFLOW> --profile docker
      • nf-core subworkflows test <SUBWORKFLOW> --profile singularity
      • nf-core subworkflows test <SUBWORKFLOW> --profile conda

@antoniasaracco
Copy link
Contributor Author

P-value plot files (*pvals_decoupler_plot.png) are no longer generated

@antoniasaracco
Copy link
Contributor Author

@suzannejin Hi! I've implemented in this PR the requested changes. Please let me know if you have any feedback or if any adjustment are needed , thanks!

// This information is used later to determine which method to run for each input
// Also, reorganize the structure to match them with the modules' input organization

ch_input_for_other = ch_input
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you! this is great :)

input[3] = Channel.of([[], [], [], []])
input[3] = Channel.of([
['id':'Condition_genotype_WT_KO', 'variable':'Condition genotype', 'reference':'WT', 'target':'KO', 'blocking':'batch'],
[], //
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what is this for? Do you necessarily need to provide such a ch_featuresheet with empty stuff?

Copy link
Contributor

@suzannejin suzannejin Aug 29, 2025

Choose a reason for hiding this comment

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

is it because of join(ch_featureshseet, by:0)?
If so, have you tried to use remainder:true?
I recall I had to do that for the same reasons in the subworkflow "abundance_differential_filter"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it's becuase of the join. I'll try that so there is no need to modify the other tests. Thanks for the suggestion!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @suzannejin, I added remainder: true to the join:

ch_input.join(ch_featuresheet, by: 0, remainder: true)

And updated the multiMap to handle variable tuple sizes since remainder: true creates tuples with different lengths (6 elements when no featuresheet matches, 8 when it does).

Please let me know what you think!

@suzannejin
Copy link
Contributor

hi @suzannejin I could not verify the inconsistency. It only shows up in CI and I cannot reproduce it locally, so I cannot confirm whether long decimals are the reason. Locally the TSV content and md5 are stable across repeated runs with Docker and Singularity. To avoid this CI-only mismatch the tests now compare specific table values instead of a snapshot md5.

Hi @antoniasaracco, have you tried it with Conda locally? For me, the long decimal issues also happen frequently with Conda.

@suzannejin
Copy link
Contributor

Hello @antoniasaracco, sorry for the delay, and thank you for addressing my comments!
I have just left two more related to the questions I asked before.
Once these are solved I think it should be done :)

@antoniasaracco
Copy link
Contributor Author

Hi @suzannejin , I've implemented the requested changes. Regarding the decoupler test, I also tried with conda locally - tested all profiles (docker, singularity, conda) and couldn't reproduce the inconsistency. The issue seems to be CI-specific.

Please let me know if you have any feedback or if any adjustment are needed , thanks!

Copy link
Member

@pinin4fjords pinin4fjords left a comment

Choose a reason for hiding this comment

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

Sorry, but let's keep things really simple and assume the feature sheet is supplied correct as diff. ab does.

I also have other ideas for simplifying this subworkflow, but probably something for a separate PR

Comment on lines 43 to 48
.join(ch_featuresheet, by: 0, remainder: true)
.multiMap { tuple ->
def (meta, file, genesets, background, method) = tuple[0..4]
def features_sheet = tuple.size() > 5 ? tuple[5] : null
def features_id = tuple.size() > 6 ? tuple[6] : ''
def features_symbol = tuple.size() > 7 ? tuple[7] : ''
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.join(ch_featuresheet, by: 0, remainder: true)
.multiMap { tuple ->
def (meta, file, genesets, background, method) = tuple[0..4]
def features_sheet = tuple.size() > 5 ? tuple[5] : null
def features_id = tuple.size() > 6 ? tuple[6] : ''
def features_symbol = tuple.size() > 7 ? tuple[7] : ''
.join(ch_featuresheet)
.multiMap { meta, file, genesets, background, method, features_sheet, features_id, features_symbol ->

I think this is over-engineered. Let's just assume the workflow is being called as differentialbundance is calling it- with a predictable feature sheet structure.

background:
[ meta_with_method, background ]
features:
[ meta_with_method, features_sheet ?: [], features_id, features_symbol]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[ meta_with_method, features_sheet ?: [], features_id, features_symbol]
[ meta_with_method, features_sheet, features_id, features_symbol]

similarly here

Copy link
Contributor

@suzannejin suzannejin left a comment

Choose a reason for hiding this comment

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

okay, it looks good to me so far. Let's see what @pinin4fjords says

Copy link
Member

@pinin4fjords pinin4fjords left a comment

Choose a reason for hiding this comment

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

Much neater now, thank you

@antoniasaracco antoniasaracco added this pull request to the merge queue Sep 16, 2025
Merged via the queue into nf-core:master with commit 4ac8159 Sep 16, 2025
47 checks passed
@antoniasaracco antoniasaracco deleted the decoupler branch September 16, 2025 17:55
@antoniasaracco antoniasaracco restored the decoupler branch September 17, 2025 00:10
antoniasaracco added a commit that referenced this pull request Sep 17, 2025
This reverts commit 4ac8159.
chaochaowong pushed a commit to chaochaowong/modules that referenced this pull request Sep 30, 2025
* update snapshot

* exchange gtf file for annot tsv file

* remove parse_gtf function

* exchange gtf file for annot tsv file

* update

* update

* update meta.yml

* fix

* Include annotation file expected format

* Update: reference file

* include decoupler to subworkflow

* Include decoupler test config

* Include decoupler to test file

* Update snapshot with decoupler output files

* Add decoupler config

* Replace gtf annotation file for tsv table for decoupler

* Update snapshot to remove pvals plots for decoupler

* include decoupler to subworkflow meta.yml

* fix syntax error

* add decoupler/decoupler tag

* fix syntax issue

* fix component

* Upgrade decoupler output assertion

* Improve format validation for annotation tsv file

* Include meta for all decoupler inputs

* Update decoupler test with new meta input requirement

* Fix decoupler input channels

* Update decoupler prefix

* Update decoupler test with new input channels

* Update snapshot

* add meta for net and annot inputs

* Include method differential to stub results to avoid null names

* Add method differential to stub results to avoid null

* Include ch_featuresheet into ch_input_for_other

* Separate channel  for other and channel for decoupler including features to not disturb other profiles tests

* update decoupler test

* Remove hardcode of gene_id and gene_name columns

* update decoupler meta.yml

* update decoupler test

* Fix: Remove features id and symbol from decoupler meta.yml

* Fix: Remove features id and symbol from decoupler input

* Fix: Take features id and symbol as external arguments

* Fix: Take features id and symbol as external arguments

* Fix: Take features id and symbol as external arguments

* Fix: remove ch_input_for_decoupler

* correct method

* Update: update nf-test to work with new input channel

* Fix: change method

* Fix: update ch_input_for_other channel to include featuresheet only for decoupler

* Update snapshot

* Fix

* Fix: Simplify features form input channel

* Update snapshot
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