search

qiime2 / q2-dada2

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

IMP: Add new percentage-merged column to denoise-paired output #120

Closed Oddant1 closed 4 years ago

Oddant1 commented 5 years ago

Closes #115. Adds a new percenteage-merged column caclulated as merged / denoised * 100 to the output from denoise_paired. Currently the 3 tests affected by this are failing, and I am unsure why. I changed the test data files accordingly. When I save the metadata output by denoise_paired to disk as a .tsv, then import it into ipython as a dataframe and compare it to the test data file imported as a dataframe, they are equal, but for some reason they are not equal when compared in the actual tests. When I checked the values of the expected and resultant dataframes in the tests, I found they only showed 7 decimals and not the 13 I see when I test them in ipython, but all decimals displayed were identical in spite of the supposed inequality.

Oddant1 commented 5 years ago

Unsurprisingly, these failures seem to be caused by some floating point shenanigans. I will look into it more tomorrow.

Oddant1 commented 5 years ago

The commit https://github.com/qiime2/q2-dada2/pull/120/commits/c6e47efaf431d30110ea5915e1316168a31413b0 arbitrarily rounds the float values to the nearest two decimal places which unsurprisingly fixes the floating point shenanigans, I don't know if it's what we want though.

ebolyen commented 4 years ago

I think this strategy is fine, but I do wonder if we shouldn't have a nicer visualization for this output at this point. It's a fairly fundamental part of our workflow...

nbokulich commented 4 years ago

@Oddant1 do you have an example QZV that I can look at for a UX review?

thermokarst commented 4 years ago

this is a QZV (with a zip extension for GH). denoising-stats.zip

thermokarst commented 4 years ago

Hey @nbokulich, take a look at this latest QZV. I think the percentage-of-input-filtered is confusing, it seems like it would be more clear if the value in that column was (input - filtered )/ input. What do you think?

nbokulich commented 4 years ago

Works for me!

thermokarst commented 4 years ago

Sounds good --- @Oddant1, I have a handful of changes I am working on here, I will ping you once those are ready.

thermokarst commented 4 years ago

Okay @nbokulich, :basketball: :+1:

QZV

Oddant1 commented 4 years ago

Those three separate commits have to do with some weirdness in attempting to merge my code with Matt's commit, but the final commit should contain what we want