ENH: Updates to cluster_features_de_novo #11
Conversation
|
@gregcaporaso - your commit messages look like this PR should be rebased, not squashed, when approved. Can you confirm when you get a chance? Thanks! |
There was a problem hiding this comment.
Thanks @gregcaporaso! This looks pretty sweet - I had a few super minor questions and comments. Thanks!
| constructor=skbio.DNA, format='fasta')) | ||
| exp_seqs = list(skbio.io.read(str(input_sequences), | ||
| constructor=skbio.DNA, format='fasta')) | ||
| print(obs_seqs) |
There was a problem hiding this comment.
Can you drop this please?
| exp_seqs = list(skbio.io.read(str(input_sequences), | ||
| constructor=skbio.DNA, format='fasta')) | ||
| print(obs_seqs) | ||
| print(exp_seqs) |
There was a problem hiding this comment.
Can you drop this please?
| skbio.io.read(sequences_fp, constructor=skbio.DNA, | ||
| format='fasta')} | ||
| feature_ids_table = set(table.ids(axis='observation')) | ||
| non_overlapping_ids = feature_ids_seqs ^ feature_ids_table |
q2_vsearch/plugin_setup.py
Outdated
| 'sequences': 'The sequences corresponding to the features in table.', | ||
| }, | ||
| parameter_descriptions={ | ||
| 'id': 'The percent identity at which clustering should be performed.', |
There was a problem hiding this comment.
I know the VSEARCH flag is --id, but how would you feel about identity here? I keep seeing this and thinking of "identifier" instead of "identity." It isn't a showstopper for me, just thought I would ask.
There was a problem hiding this comment.
I went back and forth on that... I ended up going with id to match the vsearch parameter name. In this case it's probably fine to not match vsearch, but in general I was thinking matching vsearch's parameter names might be a useful strategy, so users can easily refer to the vsearch documentation. It's really likely that we're going to expose other vsearch parameters as well, so it will help to easily be able to find info in their docs.
I'm fine either way for this one - you call it @thermokarst.
There was a problem hiding this comment.
Just chatted IRL with @jairideout about this - maybe it makes sense to match the perc-identity parameter in classify-consensus-vsearch and classify-consensus-blast, and just including a note in the description saying something like "This parameter maps to the --id flag in vsearch"
|
@thermokarst, this addresses your comments minus the name change to the |
|
@thermokarst, this one should be good to go. |
thermokarst
changed the title
Addressing #12 will improve the test coverage, but we can do that in another PR since the
.ucparser will likely change a little bit. We should be able to use one.ucparser for all of the methods we're currently planning on implementing.