Add additional alignment strategy parameters by fethalen · Pull Request #92 · qiime2/q2-alignment

fethalen · 2025-10-30T14:56:34Z

This PR addresses issue #91 by exposing the following set of additional alignment strategy parameters:

globalpair
localpair
genafpair
maxiterate
retree
nofft
auto

These flags allows the user to choose between different alignment algorithms implemented in MAFFT. There is also the --auto flag, which automatically decides on the most appropriate algorithm depending on the input data size.

globalpair, localpair, and genafpair are mutually exclusive but the default behavior of MAFFT is to just pick one based on its own internal logic. I've chosen to retain this behavior in the q2-alignment implementation, but a question I have is whether providing more than one of these flags should raise a warning or an error.

lizgehret · 2025-10-30T18:21:27Z

Thanks @fethalen! Is this one ready for a review?

fethalen · 2025-10-31T07:16:06Z

Thanks @fethalen! Is this one ready for a review?

Yes, it would be great if someone could have a look at it!

And for the person who does, please consider my question whether we should implement our own logic to handle mutually exclusive parameters or if we should just let MAFFT do its thing.

lizgehret · 2025-11-05T15:42:55Z

Sounds great, thanks @fethalen! We'll get someone to take a look at this either this week or next 🙂

fethalen · 2025-12-19T08:46:38Z

Hi @lizgehret, I'm going on a Christmas break soon, and it would be great if someone could have a look at this PR.

lizgehret · 2025-12-19T16:12:23Z

Hey @fethalen, sorry for the delay on this- I'm starting my OOO in preparation for the big move, but I just pinged the rest of the team to see if anyone has bandwidth either today or Monday!

cherman2

Hi @fethalen,

Thanks for your work on this!

Our major concern is that 'localpair', 'globalpair', 'auto', and 'genafpair' seem like mutually exclusive algorithms. I am pretty sure that MAFFT allows you to pass in all the parameters, but under the hood, it selects the last parameter passed in and uses that.

I see two ways to approach this, so our users understand they can't pass in 2 different algorithms into MAFFT.

Instead of seperate 'localpair', 'globalpair', 'auto', and 'genafpair' params, we could have a "alignment-strategy" param with choices of 'localpair', 'globalpair', 'auto', and 'genafpair'. This would make it obvious to users that these are mutually exclusive selections, but diverges from MAFFTS syntax. I think this is our preference because it seems kinda fool-proof
We can keep these parameters separate, but we should error if they are passed in together. I think this is a more complex approach, but it does mimic MAFTT's syntax. If you feel really strongly that we should keep MAFFT's syntax, I think this is a fine solution.

Let me know what you think!

fethalen · 2026-01-12T12:57:40Z

Hi @fethalen,

Thanks for your work on this!

Our major concern is that 'localpair', 'globalpair', 'auto', and 'genafpair' seem like mutually exclusive algorithms. I am pretty sure that MAFFT allows you to pass in all the parameters, but under the hood, it selects the last parameter passed in and uses that.

I see two ways to approach this, so our users understand they can't pass in 2 different algorithms into MAFFT.

Instead of seperate 'localpair', 'globalpair', 'auto', and 'genafpair' params, we could have a "alignment-strategy" param with choices of 'localpair', 'globalpair', 'auto', and 'genafpair'. This would make it obvious to users that these are mutually exclusive selections, but diverges from MAFFTS syntax. I think this is our preference because it seems kinda fool-proof

We can keep these parameters separate, but we should error if they are passed in together. I think this is a more complex approach, but it does mimic MAFTT's syntax. If you feel really strongly that we should keep MAFFT's syntax, I think this is a fine solution.

Let me know what you think!

Hi @cherman2, and thank you for looking into this PR! I agree that adding an additional parameter with all of the alignment strategies is the best solution. I've now implemented that under the parameter strategy, and updated all of the test. Please, if you have time, have a look at the updated version of my PR. Thanks!

cherman2 · 2026-01-12T16:22:54Z

Hi @fethalen, I will review this today. Thank you!

cherman2

Hi @fethalen,
One more small request. Let me know when you are done, and we will review ASAP.

cherman2 · 2026-01-12T19:26:23Z

+    valid_strategies_str = ", ".join(
+        [s for s in valid_strategies])
+
+    if strategy == "auto":
+        return ["--auto"]
+    if strategy == "fftns":
+        return []
+    if strategy == "nofft":
+        return ["--nofft"]
+    if strategy == "localpair":
+        return ["--localpair"]
+    if strategy == "globalpair":
+        return ["--globalpair"]
+    if strategy == "genafpair":
+        return ["--genafpair"]
+    if strategy is None:
+        return []
+    else:
+        raise ValueError(
+            f"Invalid alignment strategy '{strategy}'. "


We could avoid this code if we used the 'Choices' predicate. Docs are here https://develop.qiime2.org/en/stable/plugins/references/api/types.html#qiime2.plugin.Choices. That way, we can let the framework validate and yell at the user lol.

Then we can prepend a '--' to the strategy param. Ex: 'auto' to '--auto'

We could avoid this code if we used the 'Choices' predicate. Docs are here https://develop.qiime2.org/en/stable/plugins/references/api/types.html#qiime2.plugin.Choices. That way, we can let the framework validate and yell at the user lol.

Then we can prepend a '--' to the strategy param. Ex: 'auto' to '--auto'

Thanks for the suggestion. I've removed the parsing function and replaced it with Choices. The only thing that is different now is that fftns is no longer an option. This simplifies the code a bit and it is also more consistent with how MAFFT works.

cherman2 · 2026-01-12T19:27:43Z

+    if strategy:
+        strategy_flag = _validate_alignment_strategy(strategy)
+        cmd += strategy_flag


Since Choices will handle the validation, we can probably get rid of the _validate_alignment_strategy function and prepend here instead.

cherman2

Hi @fethalen,
One more thing! Thanks for your work.

cherman2 · 2026-01-13T17:09:31Z

+    "maxiterate": Int,
+    "retree": Int,


@fethalen,
We were so focused on the alignment strategy that we didn't notice that the same principles could be applied here.

Let's use the. 'Range' predicate here because MAFFT doesn't accept Negative numbers, and 'Range' will safeguard against that. https://develop.qiime2.org/en/latest/plugins/references/api/types.html#qiime2.plugin.Range

cherman2 · 2026-01-13T17:14:50Z

+
+    # --maxiterate is set to 0 by default, so we only pass this argument onto
+    # MAFFT if it deviates from this value.
+    if maxiterate not in (None, 0):
+        cmd += ['--maxiterate', str(maxiterate)]
+
+    # --retree is set to 2 by default, so we only pass this argument onto
+    # MAFFT if it deviates from this value.
+    if retree not in (None, 2):
+        cmd += ['--retree', str(retree)]
+


Let's pass in the defaults to MAFFT.

I am worried about an edge case here where MAFFT changes its defaults, and we don't notice. Then our code would be misleading and say the default is 2 or 0, but it would really be using whatever MAFFT changed its default to.

Kind of a long shot because MAFFT feels pretty stable, but this way we always control what the default is.

fethalen · 2026-01-14T09:20:42Z

Hi @cherman2, great suggestion. I've limited --p-maxiterate and --p-retree to 0-∞. I also pass on all values, even if they match the current default.

cherman2

Looks Great To Me! Let's Get This Merged!

Add additional alignment strategy parameters

bbcfc21

q2d2 added this to QIIME 2 - Triage 🚑 Oct 30, 2025

github-project-automation Bot moved this to Needs Triage in QIIME 2 - Triage 🚑 Oct 30, 2025

lizgehret removed this from QIIME 2 - Triage 🚑 Oct 30, 2025

lizgehret added this to 2026.1 ❄️ Oct 30, 2025

lizgehret moved this to Needs Review in 2026.1 ❄️ Oct 30, 2025

lizgehret linked an issue Oct 30, 2025 that may be closed by this pull request

Expose additional MAFFT alignment strategy parameters #91

Closed

lizgehret assigned cherman2 and Oddant1 Jan 6, 2026

cherman2 requested changes Jan 8, 2026

View reviewed changes

cherman2 assigned fethalen and unassigned Oddant1 and cherman2 Jan 8, 2026

cherman2 moved this from Needs Review to In Development in 2026.1 ❄️ Jan 8, 2026

Move alignment strategy flags to single flag, strategy

6ed5d57

fethalen requested a review from cherman2 January 12, 2026 14:39

cherman2 moved this from In Development to Needs Review in 2026.1 ❄️ Jan 12, 2026

Oddant1 assigned Oddant1, cherman2 and fethalen and unassigned fethalen, Oddant1 and cherman2 Jan 12, 2026

Oddant1 moved this from Needs Review to In Development in 2026.1 ❄️ Jan 12, 2026

cherman2 requested changes Jan 12, 2026

View reviewed changes

fethalen added 2 commits January 13, 2026 14:01

Simplify strategy param parsing

eb83728

Update param descriptions to reflect changes

38fb3a8

fethalen requested a review from cherman2 January 13, 2026 14:16

cherman2 requested changes Jan 13, 2026

View reviewed changes

fethalen added 2 commits January 14, 2026 10:01

Limit maxiterate and retree params to range

5105778

Update maxiterate and retree defaults; remove affected tests

e4180db

fethalen requested a review from cherman2 January 14, 2026 09:20

cherman2 approved these changes Jan 14, 2026

View reviewed changes

cherman2 merged commit d2bbfe1 into qiime2:dev Jan 14, 2026
4 checks passed

cherman2 moved this from In Development to Changelog Needed in 2026.1 ❄️ Jan 14, 2026

fethalen deleted the alignment-strategy branch January 15, 2026 09:18

Oddant1 moved this from Changelog Needed to Completed in 2026.1 ❄️ Jan 26, 2026

Uh oh!

Conversation

fethalen commented Oct 30, 2025

Uh oh!

lizgehret commented Oct 30, 2025

Uh oh!

fethalen commented Oct 31, 2025

Uh oh!

lizgehret commented Nov 5, 2025

Uh oh!

fethalen commented Dec 19, 2025

Uh oh!

lizgehret commented Dec 19, 2025

Uh oh!

cherman2 left a comment

Choose a reason for hiding this comment

Uh oh!

fethalen commented Jan 12, 2026

Uh oh!

cherman2 commented Jan 12, 2026

Uh oh!

cherman2 left a comment

Choose a reason for hiding this comment

Uh oh!

cherman2 Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

fethalen Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

cherman2 Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

cherman2 left a comment

Choose a reason for hiding this comment

Uh oh!

cherman2 Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

cherman2 Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

fethalen commented Jan 14, 2026

Uh oh!

cherman2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants