Skip to content

ENSGENOMIO-18 Chunking modules#34

Open
markquintontulloch wants to merge 45 commits into
mainfrom
ENSGENOMIO-18
Open

ENSGENOMIO-18 Chunking modules#34
markquintontulloch wants to merge 45 commits into
mainfrom
ENSGENOMIO-18

Conversation

@markquintontulloch

@markquintontulloch markquintontulloch commented Mar 11, 2026

Copy link
Copy Markdown

Creating draft pull request as following changes required:

Container version for Genomio needs to be updated once python code approved, merged, and new release container created.

Tests need to be updated in accordance with group's new policy. Tests now updated to avoid use of stored files

@markquintontulloch markquintontulloch marked this pull request as ready for review May 12, 2026 08:41

@Dishalodha Dishalodha left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partial review, will test the code when cluster is back.

Looks good overall, just minor updates.

Comment thread modules/ensembl/fasta/recombine/environment.yml Outdated
Comment thread modules/ensembl/fasta/recombine/main.nf Outdated
Comment thread tests/config/nextflow.config Outdated
Comment thread modules/assets/NO_FILE Outdated
Comment thread modules/ensembl/fasta/recombine/tests/main.nf.test
Comment thread modules/ensembl/features/combine_json/main.nf Outdated
Comment thread tests/conftest.py Outdated
Comment thread modules/ensembl/fasta/recombine/environment.yml Outdated
Comment thread modules/ensembl/fasta/recombine/main.nf Outdated
Comment thread modules/ensembl/fasta/split/main.nf Outdated
Comment thread modules/ensembl/fasta/split/environment.yml Outdated
Comment thread modules/ensembl/fasta/recombine/main.nf Outdated
Comment thread modules/ensembl/fasta/recombine/main.nf Outdated
Comment thread modules/ensembl/fasta/split/main.nf Outdated
Comment thread modules/ensembl/fasta/split/main.nf Outdated

@JAlvarezJarreta JAlvarezJarreta left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Leaving the update of the GenomIO container when the new version is released aside, looks good to me. A couple of suggestions and comments.

Comment thread .gitignore Outdated
Comment on lines +37 to +51
def args = []

if (params.chunk_id_regex) {
def rx = params.chunk_id_regex.replace("'", "'\"'\"'")
args << "--chunk-id-regex '${rx}'"
}

if (params.allow_revcomp) {
args << "--allow-revcomp"
}

def has_agp = agp && agp.baseName != 'NO_FILE'
if (has_agp) {
args << "--agp-file ${agp}"
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Whilst I'm happy with using Nextflow's native language, I'm wondering how portable would this module be if we decided to move away from Nextflow compared to having this code in Bash inside the code block. In a way seeing this reminds me of our Perl code embedded inside eHive modules.

More a shared thought than an action to be made, honestly.

Comment thread modules/ensembl/fasta/recombine/main.nf Outdated
Comment thread modules/ensembl/fasta/recombine/meta.yml Outdated
Comment thread modules/ensembl/fasta/recombine/meta.yml Outdated
Comment thread modules/ensembl/features/combine_json/main.nf Outdated
Comment thread modules/ensembl/features/combine_json/main.nf Outdated
Comment thread modules/ensembl/features/combine_json/meta.yml Outdated
Comment thread modules/ensembl/features/combine_json/meta.yml Outdated
Comment thread modules/ensembl/features/combine_json/meta.yml Outdated

@JAlvarezJarreta JAlvarezJarreta left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All good now 👍

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.

3 participants