search

qiime2 / q2-dada2

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

Dada2 restructuring #148

Closed jordenrabasco closed 2 years ago

jordenrabasco commented 2 years ago

This pull request addresses https://github.com/qiime2/q2-dada2/issues/147

This pull request restructures the R and Python code to facilitate future changes and additions to functionality.

All of the R code has been condensed to one R file to enhance code sharing between the different tech-specific read types. Optional arguments were utilized instead of positional arguments when variables are being passed from python to R scripts.

jordenrabasco commented 2 years ago

Hi All! I think that the code in this pull request is at a stage where it is ready to be pulled in. Is there a way to initiate a code review or the next step in the process? Thanks!!

thermokarst commented 2 years ago

Hi @jordenrabasco - review for this is on our radar. We're pretty backed up right now with end-of-semester related activities, but hopefully someone will get a chance to take a peek in the next week or two. Thanks!

lizgehret commented 2 years ago

One final comment @jordenrabasco - looks like some .Rhistory files made their way into this PR as well - please delete those, and I'd also recommend adding .Rhistory to the .gitignore file to prevent any accidental additions of those files in the future. Thanks!

ebolyen commented 2 years ago

Thanks everyone!

@jordenrabasco we're going to end up with a merge conflict with @leasi's #146.

Would you prefer we merge #146 first which can then be refactored, or are those changes already reasonably encompassed by this PR?

jordenrabasco commented 2 years ago

Thanks so much for taking the time to review the pull request and for all the comments @lizgehret! I will go through the comments/changes ASAP, and thanks for the suggestion on the .gitignore file, I had no idea that it was in the .Rhistory file was in the request as well.

jordenrabasco commented 2 years ago

@ebolyen The functionality of #146 isn't encompassed by the #148 associated PR. However after looking through #146, the functionality can be easily added on my end as I have recently done something similar with the "DETECT_SINGLETONS" option in dada2 (which won't be included in the #148 PR). So with that said I think whatever works best on your end, I can make work on mine!

benjjneb commented 2 years ago

@ebolyen The purpose of this refactoring is to make adding parameters like allowOneOff in #146 easier and more streamlined. Once it's in, we're planning on expose a few more parameters to the plugin that are particularly useful for long reads.

So, pulling this PR in and then re-adding allowOneOff using the new parameter structure is probably the easiest solution. That said, I don't want @leasi contribution to go unattributed, which is the potential tradeoff there.

jordenrabasco commented 2 years ago

Sorry @lizgehret just a quick question. Would you prefer that I push all of the corrections in one go or split it up somehow?

lizgehret commented 2 years ago

Sorry @lizgehret just a quick question. Would you prefer that I push all of the corrections in one go or split it up somehow?

All in one go is fine, thanks @jordenrabasco! 🙂

ebolyen commented 2 years ago

Thanks @jordenrabasco and @benjjneb!

I would like to err on the side of attribution as well, so unless @leasi has a different preference, let's merge their PR #146 first. Afterwards if things get too tangled up, we would be happy to help with any merge conflicts that arise (if assistance is needed).

leasi commented 2 years ago

Hi there! No preference on my end, please do what is more convenient for you to implement. Thank you for being mindful about attribution, let me know if I can help further :)

lizgehret commented 2 years ago

Hey @leasi, I went ahead and merged your PR #146 - thanks again for your work on that!

@jordenrabasco @benjjneb looks we've got a few merge conflicts (as promised) to clean up with this PR now - let us know if you need any assistance from @ebolyen or myself in getting those resolved!

jordenrabasco commented 2 years ago

Hey @lizgehret I just finished up implementing the changes you suggested previously. I also went ahead and added the functionality from PR #146 into this PR (in a refactored format) in the latest push, however, github is still saying that there are conflicts. Do I need to format things in a different way or is there a way around this? Thanks so much and sorry if this is a basic question!

ebolyen commented 2 years ago

Hey @jordenrabasco (@lizgehret will be out of office until Tuesday),

You'll need to create a merge commit by doing something like git pull upstream master which will produce a bunch of merge conflicts which then need to be manually resolved.

This is a super routine thing (and once you're used to it, super convenient), but is also unfortunately one of the most terrifying parts of git if you're not used to it. You will essentially go from working code to completely mangled nonsense with brackets everywhere which looks very much like something has gone terribly wrong (it hasn't, and it's normal).

If you would like, I can walk you through the process over a zoom call. I should be reasonably free tomorrow before noon MST/PDT.

jordenrabasco commented 2 years ago

Hey @ebolyen yes that would be awesome, thanks so much! I just sent you an email to work out the specifics!