qiime2 / q2-dada2

QIIME 2 plugin wrapping DADA2
BSD 3-Clause "New" or "Revised" License
19 stars 36 forks source link

Support allowOneOff option in removeBineraDenovo() #146

Closed leasi closed 2 years ago

leasi commented 2 years ago

Support of the allowOneOff argument in the runBimeraDenovo() function, which enables the user to allow for one mismatch or indel in bimera detection when set to True (default = False). An example of the impact of this parameter is described in benjjneb/dada2#610. Resolves #126.

Took the opportunity to correct the parameters description & order and command example in the header of the R scripts.

(Note: this is my first pull request ever so hopefully I did everything properly😊 )

ebolyen commented 2 years ago

Hi @leasi!

Thank you for this PR and great job on your first one, I've just approved CI to run and it looks like there's some minor lint errors. You should be able to view the log for more information on that.

Overall this all looks in order, but I do want to take a closer look tomorrow, since there's so many parameters. Thank you for updating the "documentation" in the R script as well. It looks like that got out of sync at some point :)

leasi commented 2 years ago

Hello @ebolyen, thank you for the quick and lovely feedback! I corrected the formating errors so hopefully lint will no longer complain. Let me know if there's anything else I can do to help!

Keegan-Evans commented 2 years ago

@leasi, it looks like there are some test failures, but @lizgehret and I are going to look into it in the morning!

leasi commented 2 years ago

Definitely an overlook on my end, thanks for the quick fix @lizgehret!

Keegan-Evans commented 2 years ago

An easy thing to overlook! If @lizgehret can approve in the "change requested" has been resolved, I think @ebolyen can get this merged next week when he gets back to the office. Thanks @leasi!

ebolyen commented 2 years ago

This is great, thanks everyone!

I have one more minor comment/clarification on the language of the help text (this exists in DADA2's documentation as well).

I read your thread and it sounds like allowOneOff changed from TRUE to FALSE, resulting in more "suspected bimeras" making it through the filtering. It also sounds like the new default should indeed be false.

So I take it that the "allow" is in reference to the selection of a bimera. i.e. a "bimeric" ASV which is an indel or mismatch from another would be marked as bimeric if allowOneOff=TRUE. This doesn't really match what the documentation says.

Looking at the history for DADA2, it looks like the documentation was incorrectly updated, as my understanding is the goal was to change the default behavior (not the argument which invokes the default behavior): https://github.com/benjjneb/dada2/commit/3750df1940b6b206f0ee96a6fcd0daa667c4bb90

Assuming my interpretation is correct, can we update the text here, as it seems to contradict itself.

cc @benjjneb


Unrelated feature, if you name a parameter with backticks: `parameter_name` (in this case `allow_one_off`) q2cli should recognize it in the text and underline it for you. That's maybe more useful when referencing a different parameter.

benjjneb commented 2 years ago

Thanks @ebolyen you are correct, that was a mistake in the DADA2 help text, for exactly the reason you diagnosed!

allowOneOff should be FALSE by default, and when TRUE causes bimeras that are one-off of a perfect. bimeric model to be identified and removed.

leasi commented 2 years ago

The devil is in the details, thanks for catching this @ebolyen, and for the confirmation @benjjneb (and for already opening the appropriate benjjneb/dada2#1518 issue in the dada2 original repo)!

I corrected the help accordingly in this PR. Took the opportunity to properly describe the parameter in the help using backticks as suggested!