search

qiime2 / q2-diversity

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

UX Bug: `beta` allows RelativeFrequency, Type errors on RF with unsupported metrics #296

Open jkibilds opened 4 years ago

jkibilds commented 4 years ago

In QIIME2-2020.8.0, the description of qiime diversity beta function says it accepts FeatureTable[Frequency | RelativeFrequency | PresenceAbsence] artefacts as input. In reality, if a FeatureTable[RelativeFrequency] artefact is passed as input, an error is generated. The command I used:

qiime diversity beta --i-table merged-features-relfreq.qza --p-metric 'braycurtis' --o-distance-matrix bc-dist-matrix.qza

The error I got:

Plugin error from diversity:

  Parameter 'table' requires an argument of type FeatureTable[Frequency]. An argument of type FeatureTable[RelativeFrequency] was passed.

Debug info has been saved to /tmp/qiime2-q2cli-err-h7d8fwcx.log

I noticed that in previous version (QIIME2-2020.6), the description of qiime diversity beta function only listed FeatureTable[Frequency] as an input option. Apparently, the description has been updated, but not the function itself.

thermokarst commented 4 years ago

Apparently, the description has been updated, but not the function itself.

The bray curtis implementation doesn't currently support FeatureTable[RelativeFrequency], but other beta metrics do, like jaccard. @ChrisKeefe, care to comment?

ChrisKeefe commented 4 years ago

Good eye, @jkibilds! Thanks for bringing this to our attention.

As @thermokarst mentioned, Bray-Curtis does not currently support relative frequency tables, but other metrics do. In order to allow beta to dispatch to metrics with looser type constraints, we opened it up to accept new FeatureTable subtypes.

So... I think it's good that beta can take various FeatureTable subytpes, and good that individual metrics won't let you pass unsupported types. The documentation could be clearer about this, though, and the error message is not very helpful.

I think this should be moved over to q2-diversity for now as an open issue - hopefully this is something I can tackle before YE2020.

thermokarst commented 4 years ago

@ChrisKeefe - Constraining the types via TypeMatch will allow this to work as-expected, and can go a long way to provided documentation, too.

ChrisKeefe commented 3 years ago

Forum xref