-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adding ONT functionality to RAW and CLEAN #133
Conversation
…y/mgs-workflow into simon-ont-raw-clean
@harmonbhasin Can you take an initial look and let me know if you see obvious flaws? Here is what the output of the MultiQC output for ONT data looks like: https://data.securebio.org/simons-notebook/posts/2024-12-08-mgs-ont-raw-eval/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks correct to me. The only thing I ask is that you confirm that the output for the length stats file looks good for single-end/paired-end short reads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that you added the ONT test to the nf-test file, but not to the yaml file that makes it run on Github Actions, please do this, thank you!
Fixed |
@harmonbhasin This is the error I get when nf-test is run on github. nf-test works locally. Can you let me know if you see what might be wrong here? Otherwise I adopted your suggestions. Once the nf-test thing is resolved this is good for review by Will.
|
@simonleandergrimm specific docker images from sequera (and even dockerhub for that matter) have given me trouble on Github Actions in the past, and my guess is that you're having a similar issue, although I have not run into this specific issue before. As of right now, the best way I know of fixing issues with docker images issues related to Github Actions is trial-and-error. The first thing I would do is try a different distribution of tidyverse, this might be a good place to start, https://hub.docker.com/r/rocker/tidyverse. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good to me! Once the docker image issue is fixed, it'll be ready for Will.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing the issue, this is good for you to look at @willbradshaw.
@simonleandergrimm based on our discussion yesterday, are you planning on adding any tests before we merge this? |
@willbradshaw I likely do not have time for this in the next few days (maybe even not until I leave for the holiday, due to Zephyr and P2RA manuscript demands). |
OK, let's drop that here then. I do want my comment on configs/read_type.config to be addressed before I merge this, though. |
…arameter for data being ONT. Added adapters to the ONT file so it doesn't break.
I dropped the ont param from read_type.config, and added the following bit to the run config files. Let me know if the wording looks good.
Still waiting for checks to run to completion. |
Ah looks like the ONT test files can't get fetched due to being on restricted. Will fix shortly. |
@willbradshaw This now passes tests. I also adapted the code to fit the newest structure of dev. Can you review it and let me know if I can merge it? |
@simonleandergrimm code looks fine, but there are conflicts with dev that need to be resolved before I can merge this. You need to merge dev into this branch and fix any conflicts. |
Done! |
Changes
Possible Todos (I don't think these should block merging)