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

feat: set gap alignment to "left" by default #1377

Merged
merged 5 commits into from
Jan 14, 2024

Conversation

ivan-aksamentov
Copy link
Member

No description provided.

Copy link

vercel bot commented Jan 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nextclade ✅ Ready (Inspect) Visit Preview Jan 12, 2024 4:25pm

@corneliusroemer
Copy link
Member

Not blocking but it would be nice if we could indicate the default value in the CLI help. Right now it says "possible values left, right" but not which one is the default.

@ivan-aksamentov
Copy link
Member Author

@corneliusroemer I made it as follows now:

      --gap-alignment-side <GAP_ALIGNMENT_SIDE>
          Whether to align gaps on the left or right side if equally parsimonious. Default: left
          
          [possible values: left, right]

@ivan-aksamentov
Copy link
Member Author

This text could be incorrect though depending on how dataset is configured, which we have no way of telling at the time of printing help text.

@corneliusroemer
Copy link
Member

Thanks, that's great! I think it's ok that pathogen json can overrides default and CLI won't show. The CLI just shows the bare default that applies in case nothing overrides.

@ivan-aksamentov
Copy link
Member Author

The unit test `pads_missing_left()` is failing: https://github.com/nextstrain/nextclade/blob/cc9e817b2195ccc39c1094800857f512ed6ad82f/packages_rs/nextclade/src/align/score_matrix.rs#L241-L294
with this error:
https://github.com/nextstrain/nextclade/actions/runs/7477824073/job/20351461502?pr=1377#step:7:398
Probably due to retying on the previous default value of the "gap alignment side" parameter ("right").

So I decided to make 2 tests instead of this one:
 - `pads_missing_left()` using the default "left" value for  "gap alignment side"  now got the new expectations. I copied them from the actual result, so they need to be scientifically checked.
 - `pads_missing_left_with_alignment_gap_right()` retained previous expectations, but is now using "right" in the  "gap alignment side".

Both tests are passing, but the expectations for the `pads_missing_left()` need to be checked by hand.
@rneher
Copy link
Member

rneher commented Jan 14, 2024

The validating the path matrices is a bit tricky, but it all looks correct to me.

@ivan-aksamentov ivan-aksamentov merged commit 5778fa6 into master Jan 14, 2024
20 checks passed
@ivan-aksamentov ivan-aksamentov deleted the feat/gap-alignment-left branch January 14, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants