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

Replace align_cfg::gap with align_cfg::gap_cost_affine #2037

Merged
merged 4 commits into from
Sep 16, 2020

Conversation

wvdtoorn
Copy link
Contributor

Fixes seqan/product_backlog#176.

Replaces seqan3::align_cfg::gap by seqan3::align_cfg::gap_cost_affine.
Deprecates seqan3::gap_scheme,seqan3::gap_open_score and seqan3::gap_score.

@wvdtoorn wvdtoorn requested review from a team and simonsasse and removed request for a team August 14, 2020 17:17
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.

Some tiny things and a suggestion why travis does not build.
Otherwise 👍

@wvdtoorn wvdtoorn force-pushed the align_cfg-gap_cost_affine branch 2 times, most recently from 7a39ccf to e21804d Compare August 18, 2020 20:22
@wvdtoorn wvdtoorn force-pushed the align_cfg-gap_cost_affine branch 3 times, most recently from 3b64234 to e33b813 Compare August 28, 2020 16:43
@codecov
Copy link

codecov bot commented Aug 28, 2020

Codecov Report

Merging #2037 into release-3.0.2 will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff               @@
##           release-3.0.2    #2037   +/-   ##
==============================================
  Coverage          97.82%   97.83%           
==============================================
  Files                264      264           
  Lines               9947     9926   -21     
==============================================
- Hits                9731     9711   -20     
+ Misses               216      215    -1     
Impacted Files Coverage Δ
...wise/detail/policy_affine_gap_recursion_banded.hpp 100.00% <ø> (ø)
...qan3/alignment/configuration/align_config_edit.hpp 100.00% <100.00%> (ø)
...ent/configuration/align_config_gap_cost_affine.hpp 100.00% <100.00%> (ø)
...qan3/alignment/pairwise/alignment_configurator.hpp 100.00% <100.00%> (ø)
...nt/pairwise/detail/policy_affine_gap_recursion.hpp 100.00% <100.00%> (ø)
...n3/alignment/pairwise/policy/affine_gap_policy.hpp 100.00% <100.00%> (ø)
...ignment/pairwise/policy/simd_affine_gap_policy.hpp 100.00% <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 5eba128...a84e3f8. 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.

Sorry that I commented before being done with the review. Here are some more tiny things 🧐😊

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, thanks 👍

@wvdtoorn wvdtoorn requested review from a team and marehr and removed request for a team September 1, 2020 06:09
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 :) My first pass-through. I asked the others if we really want to make the affine.extension_score member a strong type.

auto gap_open = g.get_gap_open_score(); // gap_open == -10
//! [gap_scheme]
seqan3::align_cfg::open_score gap_open = affine_scheme.open_score; // gap_open == -10
seqan3::align_cfg::extension_score gap = affine_scheme.extension_score; // gap == -1
Copy link
Member

Choose a reason for hiding this comment

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

Reminder for myself: I thought open_score/extension_score is an integer type and not a strong type.

CHANGELOG.md Outdated Show resolved Hide resolved
doc/tutorial/pairwise_alignment/index.md Outdated Show resolved Hide resolved
test/snippet/core/algorithm/configuration_get.cpp Outdated Show resolved Hide resolved
@wvdtoorn wvdtoorn requested a review from marehr September 3, 2020 12:07
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!

This is basically an approval, can you rebase and then reassign me?

There is one open discussion in the core team, whether

    // Accessing the members of the gap scheme
    int open = affine_cfg.open_score.get(); // open == -1
    int extension = affine_cfg.extension_score.get(); // extension == -10

or

    // Accessing the members of the gap scheme
    int open = affine_cfg.open_score; // open == -1
    int extension = affine_cfg.extension_score; // extension == -10

So that the members of the config element is a strong_type or the bare type.

@wvdtoorn wvdtoorn requested a review from marehr September 8, 2020 12:29
@marehr
Copy link
Member

marehr commented Sep 8, 2020

The members of affine_cfg shouldn't be strong types.

    // Accessing the members of the gap scheme
    int open = affine_cfg.open_score; // open == -1
    int extension = affine_cfg.extension_score; // extension == -10

We decided on this, see seqan/product_backlog#202

@wvandertoorn Is this enough for you and do you know what this means and can you make the changes? If not I can explain it to you :)

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 so much :) (Sorry for the delay 😿)

doc/tutorial/pairwise_alignment/configurations.cpp Outdated Show resolved Hide resolved
include/seqan3/alignment/scoring/gap_scheme.hpp Outdated Show resolved Hide resolved
include/seqan3/alignment/scoring/gap_scheme.hpp Outdated Show resolved Hide resolved
seqan3::align_cfg::band_fixed_size{seqan3::align_cfg::lower_diagonal{-4},
seqan3::align_cfg::upper_diagonal{4}};
// my_cfg is now of type configuration<gap, band>
// my_cfg is now of type configuration<gap_cost_affine, band_fixed_size>
Copy link
Member

Choose a reason for hiding this comment

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

👍 nice catch

@marehr marehr merged commit 3be0a8a into seqan:release-3.0.2 Sep 16, 2020
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