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

New assembler flag #4

Merged
merged 8 commits into from
Nov 30, 2021
Merged

New assembler flag #4

merged 8 commits into from
Nov 30, 2021

Conversation

jpaganini
Copy link
Contributor

Added new flag '-a' for specifying assembler tool. Only unicycler or spades are available.

If 'unicycler' is selected, coverage will not be used as a criteria for filtering potential contamination, since Unicycler already does a similar step.

plaScope.sh Outdated Show resolved Hide resolved
@duboism
Copy link
Member

duboism commented Nov 3, 2021

I don't have an opinion on the utility of the modification from a bioinformatics point of view.

However, the current proposal breaks compatibility because the -a option is mandatory. It's not a problem per se but requires some thoughts on the versioning: keeping -a mandatory would require switching to version 2.0; another possibility is to use spades as the default value.

@duboism
Copy link
Member

duboism commented Nov 3, 2021

Also, we must update README.md to mention that PlaScope can now work with other assemblers.

@jpaganini
Copy link
Contributor Author

Hi Mathieu,

I completely agree with using spades as default option in mode 2. I just modified that.

I also modified the README file.

Let me know if addtional modificaitons are required.

Cheers!

@jpaganini
Copy link
Contributor Author

Hi guys,

Are you still considering merging this request? Would you like me to do any extra modification?

@duboism
Copy link
Member

duboism commented Nov 21, 2021

Hello,

Sorry for the delay. We discussed some ideas about your contribution but I think we will merge it as-this quickly. We just want to review the README.md file before doing a new version.

Just a little task for you: we have added a Contributors section in README.md; Could you merge master in your branch and add a line to describe your work in this section ?

@jpaganini
Copy link
Contributor Author

Hi,

Thanks for the answer. I've been a bit busy myself lately. Modifications done!

@duboism
Copy link
Member

duboism commented Nov 26, 2021

Hello,

I can't see the commit(s) for your contribution. Did you push ?

@jpaganini
Copy link
Contributor Author

Sorry! I havent! It should be present now!

@duboism
Copy link
Member

duboism commented Nov 29, 2021

Thanks !

@GuilhemRoyer : if you're OK, I can merge this.

@GuilhemRoyer
Copy link
Contributor

Thanks !

@GuilhemRoyer : if you're OK, I can merge this.

Thank you @duboism, for sure I am Ok. Thank you also @jpaganini for your contribution.

@duboism duboism merged commit ccf7a75 into labgem:master Nov 30, 2021
@duboism
Copy link
Member

duboism commented Nov 30, 2021

I have merged the branch. Thanks @jpaganini ! I will release a new version as soon as we have decided for #5 .

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