search

qiime2 / q2-dada2

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

implemented fix to dada2 dropping blank samples. #139

Closed dwthomas closed 8 months ago

dwthomas commented 3 years ago

Fixes issue #136

dwthomas commented 3 years ago

Thanks @thermokarst

  1. As it stands dada2 will retain empty samples if they are not empty until the merging step of dada2, which to me seems like a bug, as you'd want all the empty samples or none. I think a flag might be a good but separate idea as I agree there are probably some folks assuming there are no empty samples, as you say.
  2. Some help navigating the unit tests would be useful! I was having trouble finding where to put/get the input for a unit test. I believe just adding another sample with no reads should demonstrate the behavior.
thermokarst commented 3 years ago

Thanks @dwthomas. So to make sure I'm following, would an alternative solution be to instead filter all empty samples from the final feature table? If so, why didn't you take that approach here?

dwthomas commented 3 years ago

I think the two behaviors that make sense are:

  1. All samples input to dada2 are retained, even with zero frequency.
  2. Samples with zero frequency after dada2 are removed.

The current behavior retains some but not all zero frequency samples, which I think is a potential source of confusion. I implemented the first because that's the behavior I want. But my argument for that behavior as the default is that you can go from that to retaining only non-zero samples with a qiime feature-table filter-samples command, but I do not know a way to add back removed samples without using the Artifact API.

thermokarst commented 3 years ago

Awesome, thanks for explaining, that makes sense to me! Okay yeah, I'm leaning towards going with the functionality proposed here. In the future I think it would be great to add in a new flag that lets us toggle the empty-sample retention, wholesale, but I agree, that is a separate issue. I'll think about tests and get back to you. Would you mind if I push the tests up to your feature branch?

dwthomas commented 3 years ago

That works great for me.

dwthomas commented 3 years ago

@thermokarst I'm doing some work on the unit tests, I figured out where the data files are.

dwthomas commented 3 years ago

Hey @thermokarst I added unit tests and a toggle as discussed. All the functionality is in the _denoise_helper function used by all single, paired and pyro so I modified the paired tests to include an empty sample and test both modes of the retain-all-samples toggle.

thermokarst commented 2 years ago

@dwthomas - there is one minor test failure - can you please resolve that when you get a chance? Thanks!

dwthomas commented 2 years ago

Looks like there is just a new dada2 flavor that needed the toggle added to it as well.

Oddant1 commented 1 year ago

@dwthomas are you still working on this PR? No pressure, just taking stock of the things we have in review for the coming release.

cherman2 commented 8 months ago

Hi @dwthomas! We are currently cleaning out our forum backlog! I fixed conflicts but it looks like there are still some errors! Do you have the bandwidth to take this pull request across the finish line? No pressure if not!

dwthomas commented 8 months ago

Hey @cherman2, I unfortunately don't have the bandwidth at least for the time being.

cherman2 commented 8 months ago

No problem! All these remaining issues look super superficial (and I may have caused them when I fixed conflicts). I will get this passing and then pull down to test locally!