Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for SampleData[Sequences] #59

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Add support for SampleData[Sequences] #59

wants to merge 1 commit into from

Conversation

wasade
Copy link
Member

@wasade wasade commented Mar 19, 2018

Fixes #49.

I wasn't sure how to add a test to this, and I didn't see a comparable test in q2-vsearch for the different input type. But happy to implement a test if recommended.

@thermokarst thermokarst self-assigned this Mar 25, 2018
@thermokarst thermokarst self-requested a review March 25, 2018 13:59
@thermokarst
Copy link
Contributor

Sorry @wasade for letting this sit for so long - I was traveling last week, but am back in the office now.

Copy link
Contributor

@thermokarst thermokarst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me! I am "requesting changes" here though, because this change wont work until we define a new transformer in q2-types that will let us do the reverse transformation defined here (QIIME1DemuxDirFmt -> SingleLanePerSampleSingleEndFastqDirFmt). @wasade, would you be able to cut a PR for that? I opened an issue here: qiime2/q2-types#176

@thermokarst
Copy link
Contributor

For posterity, here is the error when running right now (without that transformer in place):

Plugin error from deblur:

  No transformation from <class 'qiime2.plugin.model.directory_format.QIIME1DemuxDirFmt'> to <class 'q2_types.per_sample_sequences._format.SingleLanePerSampleSingleEndFastqDirFmt'>

@thermokarst thermokarst added the stat:DO-NOT-MERGE Please do not merge this until this label has been removed. label Apr 2, 2018
@ebolyen
Copy link
Member

ebolyen commented Apr 2, 2018

... and transforming to that format isn't possible since there are no quality scores 😞

So this is another situation where we really need typing.Union as it would let you do the obvious thing for the view annotation: typing.Union[QIIME1DemuxFormat, SingleLanePerSampleSingleEndFastqDirFmt] and then do an isinstance check inside the method (like a normal ad-hoc polymorphic python function).

Until then, we'd need to create a custom union-object (like what is used in demux summarize), but then you need to define transformers from everything to that, which really defeats the point of having transformers at all.

I think this situation has come up at least 2 or 3 times before this, so maybe it's time to just fix it for real and implement typing.Union?

@thermokarst
Copy link
Contributor

We discussed out-of-band and will hold off on merging this PR until we have some kind of Union option in place.

@ebolyen
Copy link
Member

ebolyen commented Nov 8, 2019

It seems like 2020.1 might be a good time for Python 3.7 (letting us use type.Union in a multiprocessing env).

I will optimistically add this to our list...

@thermokarst
Copy link
Contributor

Pushing this off to 2020.5, at the earliest.

@lizgehret
Copy link
Member

Hey @ebolyen now that we do have typing.Union available, is this something we should get merged?

@Oddant1
Copy link
Member

Oddant1 commented Oct 13, 2022

Pushing this off to 2020.5, at the earliest.

At the earliest. . . Following up on @lizgehret, @ebolyen that issue qiime2/q2-types#176 Matt mentioned is still open, is this actually fine now, and should that issue actually be closed? The issue is related to creating a transformer between the two formats, but if I'm understanding the conversation correctly with typing.Union we shouldn't even need that transformer, and that transformer isn't possible anyway.

@ebolyen ebolyen removed their assignment Feb 17, 2023
@gregcaporaso gregcaporaso self-assigned this Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat:DO-NOT-MERGE Please do not merge this until this label has been removed.
Projects
Development

Successfully merging this pull request may close these issues.

support SampleData[Sequences] as input
6 participants