qiime2 / q2-feature-table

QIIME 2 plugin supporting operations on feature tables.
BSD 3-Clause "New" or "Revised" License
2 stars 37 forks source link

filter_features doesn't work with FeatureTable[Composition] #89

Closed mortonjt closed 1 year ago

mortonjt commented 7 years ago

Questions How difficult would it be to generalize this to any FeatureTable, regardless of the underlying semantic type contents?

Comments Looks like this method is restricted to only work for specific table types.

jairideout commented 7 years ago

I think it should be pretty easy to extend, FeatureTable[Composition] just needs to be declared as an allowed semantic type on the method registration.

Currently the method only works with FeatureTable[Frequency]. The filtering methods were originally written with the intention of filtering based on integer counts (i.e. frequencies). If all the filtering operations in that method also make sense for compositional data then 👍 for supporting this table type.

ebolyen commented 7 years ago

This is also why we don't have something like FeatureTable[Any] if a method did not know about a particular variant, then we don't know for sure if that variants expectations are upheld by the method. So our expectation is that when new types come along, some minor annotation maintenance may be needed.

gregcaporaso commented 7 years ago

Is it possible to do this currently? The output type in this case would be dependent on the input type (if FeatureTable[Frequency] is input then FeatureTable[Frequency] would be the output, but if FeatureTable[Composition] is input then FeatureTable[Composition] would be the output), and I thought we couldn't support that at the moment.

jairideout commented 7 years ago

I think you're right -- @ebolyen can you confirm please?

ebolyen commented 7 years ago

@gregcaporaso is right, we need TypeMap to handle this correctly.

jairideout commented 7 years ago

Thanks for confirming! I guess we'll need to wait on typemap support, or have a separate method to specifically filter compositional tables (as a temporary solution). The underlying filtering code can probably be reused since it just operates on biom.Table objects.

mortonjt commented 7 years ago

That makes sense. Since compositional tables are generated from frequency tables, we can perform the filtering beforehand - so there is an immediate solution already in place. Just curious, what is TypeMap?

ebolyen commented 7 years ago

There isn't an immediate solution, you would have to duplicate the method at the moment. (I'm not even sure what I thought I read, but I'm answering a question that never happened... Sorry!)

TypeMap is supposed to be a type-solver such that you could say:

T = TypeMap({
    FeatureTable[Frequency]: FeatureTable[Frequency],
    FeatureTable[Composition]: FeatureTable[Composition]
}) # A shorthand for just plain remapping would be good

plugin.register_function(foo,
inputs: {'bar': T},
outputs: [('foo', T)],
...)

Given we want to handle things like filtering/merging more generically in QIIME 2 with indexes, it's not clear if it is immediately useful, as those operations would become builtins and wouldn't need a TypeMap

jairideout commented 7 years ago

There isn't an immediate solution, you would have to duplicate the method at the moment.

I think @mortonjt was describing a workaround where you first filter the FeatureTable[Frequency] using the available filtering methods, and then convert that filtered table into FeatureTable[Composition]. I'm in favor of using that workaround for now since we should have this solved in a general way within the next couple of releases.

mortonjt commented 7 years ago

The TypeMap solution sounds like it could be really nice! Might even be able to resolve the some of the semantic issues withPhylogeny and hierarchical clustering.

antgonza commented 6 years ago

Just ran into this and agree that will be nice to have.

lizgehret commented 1 year ago

Closing this as we don't have a clear plan or goal in mind that warrants adding this as an allowed type to filter features. If something comes up for this though, feel free to open up a new issue @mortonjt!