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

[FIX] Alignment score types should be signed #1891

Conversation

Irallia
Copy link
Contributor

@Irallia Irallia commented Jun 10, 2020

Resolves #1867

@Irallia Irallia added this to the Sprint 6 milestone Jun 10, 2020
@Irallia Irallia self-assigned this Jun 10, 2020
@Irallia Irallia force-pushed the BUG/alignment/assert_using_an_unsigned_score_type branch from caf0a2d to 6949b77 Compare June 10, 2020 11:55
@Irallia Irallia requested review from a team and simonsasse and removed request for a team June 10, 2020 12:35
@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

Merging #1891 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1891   +/-   ##
=======================================
  Coverage   97.71%   97.71%           
=======================================
  Files         254      254           
  Lines        9568     9568           
=======================================
  Hits         9349     9349           
  Misses        219      219           
Impacted Files Coverage Δ
...n3/alignment/configuration/align_config_result.hpp 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22d1a70...b9d043f. Read the comment docs.

Copy link
Member

@simonsasse simonsasse left a comment

Choose a reason for hiding this comment

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

Looks Good ! :)

@eseiler eseiler requested review from a team and smehringer and removed request for a team June 12, 2020 13:00
Copy link
Member

@smehringer smehringer left a comment

Choose a reason for hiding this comment

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

some 💅

CHANGELOG.md Outdated
@@ -102,6 +102,8 @@ Note that 3.1.0 will be the first API stable release and interfaces in this rele
* When invoking the alignment algorithm with a user defined thread count using the `seqan3::align_cfg::parallel`
configuration element, `std::thread::hardware_concurrency()` many threads were always spawned. This is now fixed and
only the specified number of threads will be spawned ([\#1854](https://github.com/seqan/seqan3/pull/1854)).
* Using an unsigned `score_type` throws a static assert, since gaps and mismatches have negative scores and thus need a
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Using an unsigned `score_type` throws a static assert, since gaps and mismatches have negative scores and thus need a
* Using an unsigned `score_type` is prevented with a static assert, since gaps and mismatches have negative scores and thus need a

static_assert(std::is_signed_v<score_type>,
"The alignment algorithm cannot be computed with an unsigned type as the score type. If you "
"explicitly want an unsigned type as the score type, please submit a feature request explaining the "
"proposed use case on our github page.");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"proposed use case on our github page.");
"your specific use case on our github page: github.com/seqan/seqan3.git.");

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
@Irallia Irallia force-pushed the BUG/alignment/assert_using_an_unsigned_score_type branch from 6949b77 to b9d043f Compare June 15, 2020 09:41
@Irallia Irallia requested a review from smehringer June 15, 2020 12:55
@smehringer smehringer merged commit 9816dcf into seqan:master Jun 16, 2020
@smehringer smehringer added this to 🍻 Done in Module: Alignment via automation Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Module: Alignment
  
🍻 Done
Development

Successfully merging this pull request may close these issues.

Alignment scores change when setting score type
3 participants