qiime2 / q2-taxa

BSD 3-Clause "New" or "Revised" License
3 stars 29 forks source link

PERF/BUG: avoid dense representation of the FeatureTable in collapse #136

Closed wasade closed 3 years ago

wasade commented 3 years ago

This pull request fixes #135, avoiding a dense representation of the FeatureTable when performing a taxonomy collapse.

wasade commented 3 years ago

Failures are real, left out adjustments for plotting tests, fixing right now

wasade commented 3 years ago

A shortcut fix is inplace. The plotting logic makes a lot of assumptions about having a DataFrame, and the plotting code isn't designed to support 10s of thousands of samples, so it seemed reasonable to just to to/from conversations to biom.Table as needed

wasade commented 3 years ago

@thermokarst I think this does it, however I've been unsuccessful at verifying the command locally. I'm observing issues obtaining the contents of the dist directory after using both a pip install -e . and pip install ., is that the right way to install a dev plugin or am I doing something weird and out the the ordinary?

thermokarst commented 3 years ago

I think this does it

Thanks!

is that the right way to install a dev plugin or am I doing something weird and out the the ordinary?

make dev should get you what you need w.r.t. npm deps.

wasade commented 3 years ago

ah! thanks :) Can run locally now, and am seeing samples on the x-axis

thermokarst commented 3 years ago

@wasade - I pushed up some small changes that tie this PR into the existing barplot visualizer, as well. Please let me know if those edits are acceptable to you - if not we can revert them and resolve elsewhere.

wasade commented 3 years ago

@thermokarst, am :+1:, thanks!!

thermokarst commented 3 years ago

Ah bummer: https://busywork.qiime2.org/teams/main/pipelines/master-distribution/jobs/integration/builds/356

I haven't dug into this yet, but in case you get there before me, @wasade

thermokarst commented 3 years ago

Okay, I think I got this over in https://github.com/qiime2/q2-taxa/pull/141