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

Update: Include sample_name IRIDA-Next input column #23

Merged
merged 13 commits into from
Oct 17, 2024
Merged

Conversation

sgsutcliffe
Copy link
Contributor

@sgsutcliffe sgsutcliffe commented Sep 20, 2024

Modified the template for input samplesheet.csv file to include the sample_name column in addition to sample in-line with changes to IRIDA-Next update as seen with the speciesabundance pipeline and staramrnf. What this means is that the output files and the sample name will be changed to sample_name if a sample_name is called. If arborator is being locally then the sample_name can be left blank.

Made a few changes:
- sample_name special characters will be replaced with "_"
- If no sample_name is supplied in the column sample will be used
- To avoid repeat values for sample_name all sample_name values will be suffixed with sample
- Tests to check that the variety of different sample_names work with the

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Add nf-test to test new feature
  • Usage Documentation in docs/usage.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).
  • Tested out in IRIDA-Next locally

Copy link

github-actions bot commented Sep 20, 2024

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit b318dc0

+| ✅ 143 tests passed       |+
#| ❔  28 tests were ignored |#
!| ❗   4 tests had warnings |!

❗ Test warnings:

❔ Tests ignored:

  • files_exist - File is ignored: assets/nf-core-arboratornf_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-arboratornf_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-arboratornf_logo_dark.png
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • files_exist - File is ignored: lib/Utils.groovy
  • files_exist - File is ignored: lib/WorkflowMain.groovy
  • files_exist - File is ignored: lib/NfcoreTemplate.groovy
  • files_exist - File is ignored: lib/WorkflowSnvphylnfc.groovy
  • nextflow_config - Config variable ignored: manifest.name
  • nextflow_config - Config variable ignored: manifest.homePage
  • nextflow_config - Config variable ignored: params.max_cpus
  • files_unchanged - File ignored due to lint config: LICENSE or LICENSE.md or LICENCE or LICENCE.md
  • files_unchanged - File ignored due to lint config: .github/CONTRIBUTING.md
  • files_unchanged - File ignored due to lint config: .github/ISSUE_TEMPLATE/bug_report.yml
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: .github/workflows/branch.yml
  • files_unchanged - File ignored due to lint config: .github/workflows/linting.yml
  • files_unchanged - File ignored due to lint config: assets/email_template.html
  • files_unchanged - File ignored due to lint config: assets/email_template.txt
  • files_unchanged - File ignored due to lint config: assets/sendmail_template.txt
  • files_unchanged - File does not exist: assets/nf-core-arboratornf_logo_light.png
  • files_unchanged - File does not exist: docs/images/nf-core-arboratornf_logo_light.png
  • files_unchanged - File does not exist: docs/images/nf-core-arboratornf_logo_dark.png
  • files_unchanged - File ignored due to lint config: docs/README.md
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/arboratornf/arboratornf/.github/workflows/awstest.yml
  • actions_awsfulltest - actions_awsfulltest
  • pipeline_name_conventions - pipeline_name_conventions

✅ Tests passed:

Run details

  • nf-core/tools version 3.0.1
  • Run at 2024-10-16 20:29:03

@sgsutcliffe
Copy link
Contributor Author

Next I will add some nf-tests specific to a samplesheet with sample_name column to make sure it behaves as expected.

@sgsutcliffe
Copy link
Contributor Author

As this is a IRIDA-Next feature, I ran it in IRADA-Next and it runs without any issue.

Copy link
Member

@apetkau apetkau left a comment

Choose a reason for hiding this comment

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

This looks really great. Thanks so much @sgsutcliffe for your work on this 😄 . I tested out in IRIDA Next and it all works.

In addition to the comment I made in-line, I'm wondering if, for some of the output (in particular for ArborView) we could have a column for both sample and sample_name?

image

That is, to add another column in the above output for sample which is filled in with the IRIDA Next identifier.

I believe this could be implemented by adding a new value to the metadata_headers and metadata_rows to pass to the MAP_TO_TSV process:

metadata_headers = Channel.value(
tuple(
ID_COLUMN, params.metadata_partition_name,
params.metadata_1_header, params.metadata_2_header,
params.metadata_3_header, params.metadata_4_header,
params.metadata_5_header, params.metadata_6_header,
params.metadata_7_header, params.metadata_8_header)
)
// Metadata rows:
metadata_rows = input_assure.result.map{
meta, mlst_files -> tuple(meta.id, meta.metadata_partition,
meta.metadata_1, meta.metadata_2, meta.metadata_3, meta.metadata_4,
meta.metadata_5, meta.metadata_6, meta.metadata_7, meta.metadata_8)
}.toList()

What do you think? Do you have any input on this @emarinier ?

CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor

@kylacochrane kylacochrane left a comment

Choose a reason for hiding this comment

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

This looks great Steven!

@sgsutcliffe
Copy link
Contributor Author

This looks really great. Thanks so much @sgsutcliffe for your work on this 😄 . I tested out in IRIDA Next and it all works.

In addition to the comment I made in-line, I'm wondering if, for some of the output (in particular for ArborView) we could have a column for both sample and sample_name?

image

That is, to add another column in the above output for sample which is filled in with the IRIDA Next identifier.

I believe this could be implemented by adding a new value to the metadata_headers and metadata_rows to pass to the MAP_TO_TSV process:

metadata_headers = Channel.value(
tuple(
ID_COLUMN, params.metadata_partition_name,
params.metadata_1_header, params.metadata_2_header,
params.metadata_3_header, params.metadata_4_header,
params.metadata_5_header, params.metadata_6_header,
params.metadata_7_header, params.metadata_8_header)
)
// Metadata rows:
metadata_rows = input_assure.result.map{
meta, mlst_files -> tuple(meta.id, meta.metadata_partition,
meta.metadata_1, meta.metadata_2, meta.metadata_3, meta.metadata_4,
meta.metadata_5, meta.metadata_6, meta.metadata_7, meta.metadata_8)
}.toList()

What do you think? Do you have any input on this @emarinier ?

I took a shot at it. 6a2cd79 Had to modify a lot of tests to accomodate the change.

@sgsutcliffe
Copy link
Contributor Author

sgsutcliffe commented Sep 28, 2024

Not sure how this update broke things per se bcause it looks like the CI are failing due to locidex container not being loaded.

  Command error:
    Unable to find image 'mwells14/locidex:0.2.3' locally
    docker: Error response from daemon: Head "https://registry-1.docker.io/v2/mwells14/locidex/manifests/0.2.3": unauthorized: incorrect username or password.

Removed my local locidex images and re-ran nf-test test and didn't have this issue.

Copy link
Member

@apetkau apetkau left a comment

Choose a reason for hiding this comment

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

Thanks so much for all your great work @sgsutcliffe . This is amazing. Thanks for making the changes I requested/adding the sample names to ArborView 😄

There is one other comment I do have given in-line.

modules/local/buildconfig/main.nf Outdated Show resolved Hide resolved
workflows/cluster_splitter.nf Show resolved Hide resolved
Copy link
Member

@apetkau apetkau left a comment

Choose a reason for hiding this comment

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

I just tested in IRIDA Next. This all works with ArborView now. Thanks so much for all your work on this 😄

@sgsutcliffe sgsutcliffe merged commit b6f9efa into dev Oct 17, 2024
4 checks passed
@sgsutcliffe sgsutcliffe deleted the add-sample-name branch October 17, 2024 20:20
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