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.

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

Jump to bottom

adding nf-core/validation #285

Merged
merged 12 commits into from
Jul 19, 2023
Merged

adding nf-core/validation #285

merged 12 commits into from
Jul 19, 2023

Conversation

louperelo
Copy link
Contributor

Modified the necessary files to add nf-core/validation with the help of @mirpedrol

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!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/funcscan branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@github-actions
Copy link

github-actions bot commented Jun 27, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 2354a87

+| ✅ 157 tests passed       |+
!| ❗   1 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline

✅ Tests passed:

Run details

  • nf-core/tools version 2.9
  • Run at 2023-07-19 14:18:57

@mirpedrol
Copy link
Member

Some of the errors are due to the meta map in the input channel bein an ImmutableMap. This means that is a map which can’t be modified in place, because this can introduce some bugs due to nextflow process being run in parallel. This can be deactivated and go back to a normal and modifiable map, but I think it’s a good opportunity to update the code to avoid possible future problems with these map operations.

For example, this error points that you can't .put() items into an ImmutableMap.
In your code, line 152, you are adding a new value to the meta map meta['longest_contig'] = Integer.parseInt(length), which uses the method .put(). One option to allow the modification of this is map is adding the new value with the operator + which is allowed in ImmutableMaps (this operator constructs a new Map based on the original one, so it is not modifying the original map).
See an example here. I think the code in this case should look something like

def new_meta = meta + ['longest_contig': Integer.parseInt(length)]

@jfy133
Copy link
Member

jfy133 commented Jul 4, 2023

Some of the errors are due to the meta map in the input channel bein an ImmutableMap. This means that is a map which can’t be modified in place, because this can introduce some bugs due to nextflow process being run in parallel. This can be deactivated and go back to a normal and modifiable map, but I think it’s a good opportunity to update the code to avoid possible future problems with these map operations.

For example, this error points that you can't .put() items into an ImmutableMap. In your code, line 152, you are adding a new value to the meta map meta['longest_contig'] = Integer.parseInt(length), which uses the method .put(). One option to allow the modification of this is map is adding the new value with the operator + which is allowed in ImmutableMaps (this operator constructs a new Map based on the original one, so it is not modifying the original map). See an example here. I think the code in this case should look something like

def new_meta = meta + ['longest_contig': Integer.parseInt(length)]

OMg this is awesome! Ever since that was brought up and Rob's talk I've been super paranoid about this! It's really cool nf-validation will help us find these places where we could be borking things :D

@mirpedrol
Copy link
Member

It is probable that we have to unable the ImmutableMaps though, the new class has some errors when using -resume :( See discussion. But at least we can already fix this line here :)

@jfy133
Copy link
Member

jfy133 commented Jul 19, 2023

Tested:

  1. Duplicate sample IDs
  2. Duplicate FASTAs
  3. Spaces in name
  4. Duplicate lines
  5. File does not exist
  6. Added support for fna.gz

@jfy133 jfy133 requested a review from mirpedrol July 19, 2023 09:19
@jfy133
Copy link
Member

jfy133 commented Jul 19, 2023

@louperelo could you and @mirpedrol look again, if all good we can merge into dev, and then I will start work again on the ORF input

@jfy133 jfy133 mentioned this pull request Jul 19, 2023
Closed
9 tasks
mirpedrol
mirpedrol approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

LGTM :)

CHANGELOG.md Outdated Show resolved Hide resolved
workflows/funcscan.nf Outdated Show resolved Hide resolved
jfy133
jfy133 reviewed Jul 19, 2023
View reviewed changes
workflows/funcscan.nf Outdated Show resolved Hide resolved
louperelo
louperelo commented Jul 19, 2023
View reviewed changes
Copy link
Contributor Author

@louperelo louperelo left a comment

Choose a reason for hiding this comment

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

Just found a tiny typo. Otherwise all good.

assets/schema_input.json Outdated Show resolved Hide resolved
@jfy133 jfy133 merged commit 22f26b2 into dev Jul 19, 2023
@jfy133 jfy133 deleted the validation branch July 19, 2023 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfy133 jfy133 jfy133 left review comments

@mirpedrol mirpedrol mirpedrol approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@louperelo @mirpedrol @jfy133