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

Posible fix for issue #127 #128

Merged
merged 8 commits into from
May 11, 2023
Merged

Posible fix for issue #127 #128

merged 8 commits into from
May 11, 2023

Conversation

ctuni
Copy link
Contributor

@ctuni ctuni commented May 10, 2023

#127 This is a possible fix for the aforementioned issue. I tested both with -profile test,docker and with my data and infrastructure and the process that was failing on my case (VALIDATOR) is no longer failing. The test profile was not failing but it's working correctly with the fix also.

The change is made in the creation of ch_input channel, just adding file() around params.input. Apparently, this way the input matrix can be read as a file and it does not throw the error.

This change is made in line 17 of differentialabundance.nf file.

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>).

@ctuni ctuni requested review from pinin4fjords and WackerO May 10, 2023 10:16
@github-actions
Copy link

github-actions bot commented May 10, 2023

nf-core lint overall result: Passed ✅

Posted for pipeline commit 3da22bf

+| ✅ 160 tests passed       |+

✅ Tests passed:

Run details

  • nf-core/tools version 2.8
  • Run at 2023-05-11 10:59:57

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.

Looks sensible- thanks for the fix!

I’d normally object to lots of random spacing changes unrelated to the fix (presumably from your IDE), but it mostly seems to be rogue spaces being removed (@WackerO will be happy), and it’s come up before, so we’ll allow it ;-).

@pinin4fjords
Copy link
Member

Ahh, wait, @ctuni - could you add the file existence check before you merge?

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.

Just need a file existence check

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.

Just need a file existence check

@ctuni
Copy link
Contributor Author

ctuni commented May 11, 2023

HI! I'm sorry for the IDE and whitespaces thing, I did not realized it happened when pulling. I can try and re-do the PR with it but thanks for allowing it!

I added the file existence check also :)

@pinin4fjords
Copy link
Member

HI! I'm sorry for the IDE and whitespaces thing, I did not realized it happened when pulling. I can try and re-do the PR with it but thanks for allowing it!

I added the file existence check also :)

No, don’t worry, @WackerO had the same issue before, and the changes are harmless. Thanks for the fix.

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.

LGTM now

@ctuni ctuni merged commit cdd9d9b into nf-core:dev May 11, 2023
@WackerO
Copy link
Collaborator

WackerO commented May 11, 2023

Well this is brilliant :D I was asking Cris to remove the whitespaces because I sensed a disturbance in the force and predicted your objection, @pinin4fjords! But yes, I am happy ^^

@pinin4fjords pinin4fjords added this to the 1.3.0 milestone Oct 10, 2023
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