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

Feature - Make the score type configurable through the result config object #1340

Merged
merged 6 commits into from
Dec 3, 2019

Conversation

smehringer
Copy link
Member

@smehringer smehringer commented Oct 31, 2019

No description provided.

@smehringer smehringer force-pushed the alignment_misc branch 5 times, most recently from ea9cc3e to 2756e71 Compare November 7, 2019 10:33
@codecov
Copy link

codecov bot commented Nov 7, 2019

Codecov Report

Merging #1340 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1340      +/-   ##
==========================================
+ Coverage   97.52%   97.52%   +<.01%     
==========================================
  Files         226      226              
  Lines        8778     8780       +2     
==========================================
+ Hits         8561     8563       +2     
  Misses        217      217
Impacted Files Coverage Δ
...n3/alignment/configuration/align_config_result.hpp 100% <ø> (ø) ⬆️
...de/seqan3/alignment/pairwise/edit_distance_fwd.hpp 100% <ø> (ø) ⬆️
...qan3/alignment/pairwise/alignment_configurator.hpp 100% <ø> (ø) ⬆️

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 eaf0546...cc9e158. Read the comment docs.

Copy link
Member

@eseiler eseiler left a comment

Choose a reason for hiding this comment

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

💅

include/seqan3/alignment/pairwise/edit_distance_fwd.hpp Outdated Show resolved Hide resolved
Copy link
Member

@marehr marehr left a comment

Choose a reason for hiding this comment

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

Thank you for finding your way in the alignment monster-dule. I would suggest some changes to simplify the score_type extraction, based on some assumptions that we can make.

Copy link
Member

@marehr marehr left a comment

Choose a reason for hiding this comment

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

tini tiny remark

EXPECT_TRUE((std::is_same_v<decltype(std::declval<_t>().id()), uint32_t>));
EXPECT_TRUE((std::is_same_v<decltype(std::declval<_t>().score()), int32_t>));
}

{ // test case II
auto cfg = align_cfg::edit | align_cfg::result{with_score};
Copy link
Member

Choose a reason for hiding this comment

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

can't you make this one case I and ...

(or you have to change the numbering // test case II -> // test case I, // test case III)

@@ -63,6 +55,18 @@ TEST(alignment_selector, align_result_selector_with_list)
}
}

TEST(alignment_selector, align_result_selector_using_score_type)
Copy link
Member

Choose a reason for hiding this comment

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

... this one case II?

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer single tests for single cases so I just removed the documentation from above. It just states the obvious anyway..

@marehr
Copy link
Member

marehr commented Nov 30, 2019

The snippets are failing: https://travis-ci.org/seqan/seqan3/jobs/618589436#L5802

@marehr
Copy link
Member

marehr commented Dec 1, 2019

The snippet failing is still a thing.

… align_result_selector change the if clause to an static assert.
@smehringer smehringer merged commit 79d60d0 into seqan:master Dec 3, 2019
@smehringer smehringer deleted the alignment_misc branch May 29, 2020 05:47
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.

None yet

3 participants