qiime2 / q2-gneiss

QIIME2 plugin for Gneiss
BSD 3-Clause "New" or "Revised" License
0 stars 15 forks source link

error if feature table samples aren't found in metadata #25

Closed jairideout closed 5 years ago

jairideout commented 6 years ago

An error should be raised if any sample IDs in the feature table aren't present in the sample metadata. This is to match the behavior in QIIME 1 and 2 where metadata IDs can be a superset of the table IDs, but every table ID must have corresponding metadata.

forum x-ref

Oddant1 commented 5 years ago

Is this still a relevant issue? The error definitely isn't being raised, but it looks like all of the methods that even use metadata in gneiss are deprecated.

thermokarst commented 5 years ago

It might still be relevant:

mortonjt commented 5 years ago

Thanks @thermokarst - yes this is still an outstanding issue

It'll boil down to something as follows here: https://github.com/qiime2/q2-gneiss/blob/master/q2_gneiss/cluster/_cluster.py#L93

difference = set(table.index) - set(gradient.index)
if difference and not ignore_missing_samples:
            raise KeyError("There are samples not included in the mapping "
                           "file. Override this error by using the "
                           "`ignore_missing_samples` argument. Offending "
                           "samples: %s"
                           % ', '.join(sorted([str(i) for i in difference])))

Concerning deprecation - the stats methods are being deprecated in favor of other differential abundance methods such as songbird and hopefully aldex2. CC @ggloor

Oddant1 commented 5 years ago

And this should be a thing in all q2-gneiss methods taking a Metadata or MetadataColumn as input?

mortonjt commented 5 years ago

Yes - especially if it isn't deprecated

On Fri, Aug 2, 2019, 5:18 PM Oddant1 notifications@github.com wrote:

And this should be a thing in all q2-gneiss methods taking a Metadata or MetadataColumn as input?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/qiime2/q2-gneiss/issues/25?email_source=notifications&email_token=AA75VXJU7AGQLJUR6T6TQ73QCSQCDA5CNFSM4EJ5FLFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3O3RSA#issuecomment-517847240, or mute the thread https://github.com/notifications/unsubscribe-auth/AA75VXP65KZQF4HEYSMNV3TQCSQCDANCNFSM4EJ5FLFA .

Oddant1 commented 5 years ago

:laughing: those poor lonely deprecated methods

Oddant1 commented 5 years ago

I'm not finding any relevant methods other than gradient_clustering

Oddant1 commented 5 years ago

Also, do we want ignore_missing_samples arguments to be included in any relevant methods?

mortonjt commented 5 years ago

Why not - that's the parameter used in emperor. There's definitely the use case that you do want to remove non overlapping samples.

On Fri, Aug 2, 2019, 5:39 PM Oddant1 notifications@github.com wrote:

Also, do we want ignore_missing_samples arguments to be included in any relevant methods?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/qiime2/q2-gneiss/issues/25?email_source=notifications&email_token=AA75VXNXYR77IKHNNUN6AI3QCSSQ7A5CNFSM4EJ5FLFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3O4WUA#issuecomment-517851984, or mute the thread https://github.com/notifications/unsubscribe-auth/AA75VXMYMUDNJME6PORFQM3QCSSQ7ANCNFSM4EJ5FLFA .

mortonjt commented 5 years ago

Not quite - that's testing to see if all of the samples are dropped. The code previously tests to see if there is a complete overlap in samples

On Fri, Aug 2, 2019, 6:22 PM Oddant1 notifications@github.com wrote:

is this exception already doing something similar to this?

[image: image] https://user-images.githubusercontent.com/10642616/62401675-0c8f6c00-b539-11e9-9062-80f11b187b31.png

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/qiime2/q2-gneiss/issues/25?email_source=notifications&email_token=AA75VXMBSSCAJDCILHAHMJLQCSXQRA5CNFSM4EJ5FLFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3O6ZWQ#issuecomment-517860570, or mute the thread https://github.com/notifications/unsubscribe-auth/AA75VXOWHCT5SILJ4L42GPDQCSXQRANCNFSM4EJ5FLFA .

Oddant1 commented 5 years ago

Yeah I realized that after looking at it a bit more closely

ggloor commented 5 years ago

yes we are getting close to having ALDEx2 up and running inside QIIME2. https://github.com/dgiguer is just putting the finishing touches on this