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

Refactor the single_end meta thing to be pulled from the ENA metadata. #9

Merged
merged 7 commits into from
Jun 5, 2024

Conversation

mberacochea
Copy link
Member

Include the Fetch tool metadata into the multiqc report

Include the Fetch tool metadata into the multiqc report
@mberacochea mberacochea requested a review from Ales-ibt June 3, 2024 21:23
Copy link
Contributor

@KateSakharova KateSakharova left a comment

Choose a reason for hiding this comment

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

I think taking metadata from ENA is good. But we should probably add an ability to user/prefect to push correct library_strategy or library_layout to pipeline. Waiting for ENA to fix data can take time (and we had cases where they must contact users but users are unknown/do not reply..).
Basically, first iteration of pipeline should be with ENA metadata. If something failed, we checked - we should be able to specify fixed args and push assembly and do not delay it.
In my view, we also need to add library_strategy or library_layout to params

modules/local/fetchtool_reads.nf Outdated Show resolved Hide resolved
@mberacochea
Copy link
Member Author

I think taking metadata from ENA is good. But we should probably add an ability to user/prefect to push correct library_strategy or library_layout to pipeline. Waiting for ENA to fix data can take time (and we had cases where they must contact users but users are unknown/do not reply..). Basically, first iteration of pipeline should be with ENA metadata. If something failed, we checked - we should be able to specify fixed args and push assembly and do not delay it. In my view, we also need to add library_strategy or library_layout to params

Indeed.. I wil add 2 more parameters for that (same logic, if they are there then it will ignore the metadata)

Copy link
Member

@chrisAta chrisAta left a comment

Choose a reason for hiding this comment

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

Looks good to me thank you!! Just a few minor comments

modules/nf-core/fastp/main.nf Show resolved Hide resolved
nf-test.config Show resolved Hide resolved
tests/main.nf.test Outdated Show resolved Hide resolved
Copy link

@Ales-ibt Ales-ibt left a comment

Choose a reason for hiding this comment

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

It looks Ok to me, thank you.

- Make MEGAHIT fail if the contigs are empty
- MultiQC ENA Metadata report (remove the hardcoded name from the process)
- Added some more unit tests
- Fix the assembler override ( only use single_end auto selection if the assembler is null )
Copy link
Member

@Ge94 Ge94 left a comment

Choose a reason for hiding this comment

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

Nothing to say really, huge work. Thank you!

There is just one comment that could be addressed

@mberacochea
Copy link
Member Author

Perfect. I have enough 👍 to merge... :shipit:

@mberacochea mberacochea merged commit b9d8a84 into main Jun 5, 2024
1 check passed
@mberacochea mberacochea deleted the feature/minor-refactor-single-end-l-source branch June 5, 2024 08:24
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.

5 participants