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

ENH: add support for --addfragments flag #71

Merged
merged 3 commits into from
Nov 13, 2020

Conversation

misialq
Copy link

@misialq misialq commented Oct 27, 2020

Adds supports for --addfragments flag for addition of fragmentary sequences to existing alignments.

Since it's just a small variant of mafft_add, rather than creating a new action I introduced a new parameter in the existing one that will determine whether the --add flag (current behaviour) or --addfragments one should be used.

@thermokarst thermokarst added this to In Review in 2020.11 via automation Oct 27, 2020
Comment on lines 60 to 61
'fragments': 'This flag indicates that alignment optimized for '
'addition of fragmentary sequences should be used.'},
Copy link
Member

Choose a reason for hiding this comment

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

Rename fragments to short_fragments and default to False?

and rephrase to something like:
'Optimize for the addition for short sequence fragments. For example, primer or amplicon sequences'

Copy link
Contributor

Choose a reason for hiding this comment

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

I like taking the approach of ensuring the plugin parameter name matches the underlying tool flag name - so in this case, addfragments. I like to think that this helps minimize "translation" confusion when trying to figure out how a wrapper-plugin (like this one) does what it does. What do you think, @mikerobeson?

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. That makes sense to me. fragments it is. :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm proposing a third option: addfragments (instead of fragments, or short_fragments), that way the param names match!

Copy link
Member

Choose a reason for hiding this comment

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

I like addfragments. I suppose this would be defaulted False? That is it would do add by default?

Copy link
Author

Choose a reason for hiding this comment

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

Alright, changed the param name to addfragments. Not passing the param flag would default to a plain add.

Comment on lines 60 to 62
'addfragments': 'Optimize for the addition of short sequence '
'fragments (for example, primer or amplicon '
'sequences).'},
Copy link
Member

Choose a reason for hiding this comment

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

I was just wondering if we should be a tiny bit more explicit to avoid confusion:

'addfragments': 'Optimize for the addition of short sequence '
                'fragments (for example, primer or amplicon '
                'sequences). If not set, default sequence addition '
                'is used.'

Copy link
Author

Choose a reason for hiding this comment

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

True, good point - updated.

Copy link
Member

@mikerobeson mikerobeson left a comment

Choose a reason for hiding this comment

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

Everything looks fine to me. Just wanted to voice my approval. :-)

@thermokarst thermokarst self-assigned this Nov 9, 2020
@thermokarst thermokarst self-requested a review November 9, 2020 17:35
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 is fantastic, thanks so much for this @misialq!

cc @gregcaporaso

@thermokarst thermokarst merged commit c74a461 into qiime2:master Nov 13, 2020
2020.11 automation moved this from In Review to Changelog Needed Nov 13, 2020
@gregcaporaso
Copy link
Member

Thanks so much for the contribution @misialq and for the review @mikerobeson! I have a use case for this now, so very excited to try it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
2020.11
Completed
Development

Successfully merging this pull request may close these issues.

None yet

4 participants