[Alignment Configuration] Adapt alignment_configurator to use the free end gap configuration from method_global #2067

smehringer · 2020-08-20T12:04:55Z

Extracted from #2032

Only the last commit is relevant

marehr

One thing, otherwise looks fine

marehr · 2020-08-25T11:28:18Z

include/seqan3/alignment/pairwise/alignment_configurator.hpp

+        if constexpr (traits_t::is_global && !config_t::template exists<seqan3::align_cfg::aligned_ends>())
+        {
+            auto method_global_cfg = cfg.get_or(align_cfg::method_global{});
+
+            if (method_global_cfg.free_end_gaps_sequence1_leading ||
+                method_global_cfg.free_end_gaps_sequence1_trailing ||
+                method_global_cfg.free_end_gaps_sequence2_leading ||
+                method_global_cfg.free_end_gaps_sequence2_trailing)
+            {
+                // append an extra aligned_ends configuration for static configuration that is needed in
+                // configure_free_ends_optimum_search (within select_find_optimum_policy)
+                auto ends_config = seqan3::align_cfg::aligned_ends{seqan3::free_ends_none};
+                return configure_free_ends_optimum_search<function_wrapper_t, policies_t..., gap_init_policy_t>(cfg | ends_config);
+            }
+        }
+


Suggested change

if constexpr (traits_t::is_global && !config_t::template exists<seqan3::align_cfg::aligned_ends>())

{

auto method_global_cfg = cfg.get_or(align_cfg::method_global{});

if (method_global_cfg.free_end_gaps_sequence1_leading ||

method_global_cfg.free_end_gaps_sequence1_trailing ||

method_global_cfg.free_end_gaps_sequence2_leading ||

method_global_cfg.free_end_gaps_sequence2_trailing)

{

// append an extra aligned_ends configuration for static configuration that is needed in

// configure_free_ends_optimum_search (within select_find_optimum_policy)

auto ends_config = seqan3::align_cfg::aligned_ends{seqan3::free_ends_none};

return configure_free_ends_optimum_search<function_wrapper_t, policies_t..., gap_init_policy_t>(cfg | ends_config);

}

}

I tried this without it and it still compiled and passed all tests, are you sure that you still need this?

Did it?? 😆 Damn... I thought I had some test failures because of this...
And I thought Rene told me we still need the static distinction in some simd policy 🤷 But if all tests run through (and we will get rid of the old alignment code anyway) I guess I'll just remove it.

smehringer · 2020-08-27T06:09:44Z

@marehr The suggested removal cause GCC7 to fail with g++-7: internal compiler error: Killed (program cc1plus)
Since this is part of the "old alignment" code that will be removed anyway, can we use my "fix" without further investigation?
I can also make this a GCC7 workaround.

marehr · 2020-08-27T12:32:52Z

Interesting; only coverage-gcc7 ICEs, normal gcc-7 build worked

I can't re-create that ICE locally :(

marehr · 2020-08-27T12:35:02Z

@marehr The suggested removal cause GCC7 to fail with g++-7: internal compiler error: Killed (program cc1plus)
Since this is part of the "old alignment" code that will be removed anyway, can we use my "fix" without further investigation?
I can also make this a GCC7 workaround.

Let's keep your patch for now; I thought about my request and I guess that you have in all test cases aligned_ends so that it falsely works, but would be correct once we remove the aligned_ends from the test cases.

marehr

(Re-assign me if you undid the change)

smehringer · 2020-08-27T12:42:21Z

ok, thanks for the quick response.

Let's keep your patch for now; I thought about my request and I guess that you have in all test cases aligned_ends so that it falsely works, but would be correct once we remove the aligned_ends from the test cases.

Oh you are right!! This is probably why it works in this PR but I had the patch. I'll rebase.

smehringer · 2020-08-28T06:14:00Z

Damn.. The coverage error persists. I'll investigate

codecov · 2020-09-07T12:59:56Z

Codecov Report

Merging #2067 into release-3.0.2 will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@                Coverage Diff                @@
##           release-3.0.2    #2067      +/-   ##
=================================================
- Coverage          97.83%   97.82%   -0.02%     
=================================================
  Files                264      263       -1     
  Lines               9926     9920       -6     
=================================================
- Hits                9711     9704       -7     
- Misses               215      216       +1

Impacted Files	Coverage Δ
...qan3/alignment/pairwise/alignment_configurator.hpp	`97.56% <100.00%> (-2.44%)`	⬇️
include/seqan3/range/views/async_input_buffer.hpp	`98.11% <0.00%> (-0.04%)`	⬇️
include/seqan3/range/views/take.hpp	`98.46% <0.00%> (ø)`
...clude/seqan3/range/views/enforce_random_access.hpp	`100.00% <0.00%> (ø)`
...de/seqan3/argument_parser/detail/version_check.hpp	`92.74% <0.00%> (ø)`
...qan3/alignment/configuration/align_config_edit.hpp

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 3bee0fd...272ddad. Read the comment docs.

smehringer · 2020-09-07T13:01:27Z

@marehr CI seems to pass now. I asked Enrico to run his script again to get the exact RAM usage again, but it might be fine.

eseiler · 2020-09-07T13:19:46Z

Every test with more than 1000 MiB RAM usage when compiling

unit/alignment/pairwise/global_affine_unbanded_callback_test.cpp                             3080 
unit/search/search_test.cpp                                                                  2871 
unit/search/search_collection_test.cpp                                                       2702 
unit/alignment/pairwise/align_pairwise_test.cpp                                              2472 
unit/alignment/pairwise/alignment_configurator_test.cpp                                      2415 
unit/io/alignment_file/format_bam_test.cpp                                                   2146 
unit/alignment/pairwise/local_affine_banded_test.cpp                                         2066 
unit/alignment/pairwise/semi_global_affine_banded_test.cpp                                   2055 
unit/alignment/pairwise/local_affine_unbanded_test.cpp                                       1971 
unit/search/fm_index_cursor/fm_index_cursor_collection_test.cpp                              1856 
unit/io/alignment_file/alignment_file_output_test.cpp                                        1793 
unit/alignment/pairwise/semi_global_affine_unbanded_test.cpp                                 1785 
unit/io/alignment_file/format_sam_test.cpp                                                   1777 
unit/alignment/pairwise/global_affine_unbanded_collection_test.cpp                           1700 
unit/io/alignment_file/alignment_file_input_test.cpp                                         1662 
unit/range/views/view_minimiser_hash_test.cpp                                                1579 
unit/search/fm_index_cursor/bi_fm_index_cursor_test.cpp                                      1565 
unit/range/views/view_kmer_hash_test.cpp                                                     1560 
unit/search/fm_index_cursor/bi_fm_index_cursor_collection_test.cpp                           1525 
unit/alignment/pairwise/global_affine_unbanded_test.cpp                                      1523 
unit/alignment/pairwise/global_affine_unbanded_collection_callback_test.cpp                  1507 
unit/alignment/pairwise/global_affine_unbanded_collection_simd_test.cpp                      1480 
unit/alignment/pairwise/global_affine_banded_test.cpp                                        1450 
unit/search/fm_index/bi_fm_index_dna4_test.cpp                                               1437 
unit/search/fm_index/bi_fm_index_aa27_test.cpp                                               1436 
unit/search/fm_index/bi_fm_index_char_test.cpp                                               1403 
unit/alignment/pairwise/edit_distance/semi_global_edit_distance_max_errors_unbanded_test.cpp 1387 
unit/io/sequence_file/sequence_file_input_test.cpp                                           1288 
unit/io/sequence_file/sequence_file_format_sam_test.cpp                                      1286 
unit/range/views/view_minimiser_test.cpp                                                     1258 
unit/search/fm_index_cursor/fm_index_cursor_test.cpp                                         1250 
unit/alignment/pairwise/edit_distance/global_edit_distance_max_errors_unbanded_test.cpp      1242 
unit/search/fm_index/fm_index_dna4_test.cpp                                                  1173 
unit/io/sequence_file/sequence_file_format_genbank_test.cpp                                  1157 
unit/io/structure_file/format_vienna_test.cpp                                                1142 
unit/io/sequence_file/sequence_file_format_embl_test.cpp                                     1093 
unit/io/sequence_file/sequence_file_format_fastq_test.cpp                                    1054 
unit/alignment/pairwise/edit_distance/semi_global_edit_distance_unbanded_test.cpp            1034 
unit/range/views/view_translate_join_test.cpp                                                1007 
unit/search/search_scheme_algorithm_test.cpp                                                 1007

smehringer · 2020-09-10T06:23:41Z

@marehr polite ping :) if you have some time between the GCB stuff

marehr · 2020-09-15T09:43:09Z

Codecov says:

// within configure_edit_distance function
auto has_free_ends_trailing = [&] (auto first) constexpr
{
    if constexpr (!decltype(first)::value)
    {
        return configure_edit_traits(std::false_type{});
    }
    else // Resolve correct property at runtime.
    {
        if (method_global_cfg.free_end_gaps_sequence1_trailing)
            return configure_edit_traits(std::true_type{});
        else
            return configure_edit_traits(std::false_type{}); // says that this line wasn't executed; 
    }
};

The coverage report also didn't show that that line was executed (wasn't highlighted = line is neither green nor red), so that means we always didn't test that code via the public interface.

smehringer · 2020-09-15T10:00:29Z

@marehr Yes I noticed that too but since it wasn't newly introduced it was independent of this PR.
I can create a backlog item if you want? But since it is in "the old alignment code" I didn't mention this before.

marehr

Just to be sure, the memory footprint is fine now, right? Can we get a comparison between before and after?

(Enricos post wasn't completely clear which commit he benchmarked)

smehringer · 2020-09-15T10:08:03Z

Just to be sure, the memory footprint is fine now, right? Can we get a comparison between before and after?
(Enricos post wasn't completely clear which commit he benchmarked)

He benchmarked after I rebased on Renes changes and the highest memory footprint was now ~3.08GB which is only an increase of a few percent compared to before. Nothing compared to the 7-8GB in the initial PR.

rrahn

Nice, now finally we get rid of this rather complicated dispatching mechanism.
One minor thing 💅 only.

include/seqan3/alignment/pairwise/alignment_configurator.hpp

…m method global.

smehringer · 2020-09-18T05:39:38Z

Code coverage is not showing anything new that this PR introduces.

smehringer requested a review from marehr August 20, 2020 12:05

smehringer force-pushed the alignment_configuration_free_end_gaps_4 branch from 8f28f26 to b4d2ca7 Compare August 21, 2020 07:06

smehringer removed the request for review from marehr August 21, 2020 07:16

smehringer force-pushed the alignment_configuration_free_end_gaps_4 branch from b4d2ca7 to 01d5e94 Compare August 21, 2020 07:47

smehringer changed the title ~~Alignment configuration free end gaps 4~~ [Alignment Configuration] Adapt alignment_configurator to use the free end gap configuration from method_global Aug 21, 2020

smehringer requested a review from marehr August 21, 2020 08:35

marehr requested changes Aug 25, 2020

View reviewed changes

smehringer force-pushed the alignment_configuration_free_end_gaps_4 branch from ac02a31 to 0fde0ba Compare August 25, 2020 12:59

smehringer requested a review from marehr August 27, 2020 06:11

marehr reviewed Aug 27, 2020

View reviewed changes

smehringer force-pushed the alignment_configuration_free_end_gaps_4 branch from 0fde0ba to 30d6f46 Compare August 27, 2020 12:46

smehringer force-pushed the alignment_configuration_free_end_gaps_4 branch from 30d6f46 to e276071 Compare September 7, 2020 12:43

smehringer requested review from marehr and removed request for marehr September 7, 2020 12:44

smehringer requested a review from marehr September 7, 2020 13:00

marehr approved these changes Sep 15, 2020

View reviewed changes

smehringer requested a review from rrahn September 16, 2020 09:43

smehringer force-pushed the alignment_configuration_free_end_gaps_4 branch from e276071 to fbbff06 Compare September 17, 2020 06:35

rrahn requested changes Sep 17, 2020

View reviewed changes

include/seqan3/alignment/pairwise/alignment_configurator.hpp Show resolved Hide resolved

smehringer added 2 commits September 17, 2020 15:25

[MISC] Adapt alignment configurator to free end gap configuration fro…

aa4ff59

…m method global.

[MISC] Sort includes alphabetically.

272ddad

smehringer force-pushed the alignment_configuration_free_end_gaps_4 branch from fbbff06 to 272ddad Compare September 17, 2020 13:26

smehringer requested a review from rrahn September 17, 2020 13:26

rrahn approved these changes Sep 18, 2020

View reviewed changes

rrahn merged commit 31fc231 into seqan:release-3.0.2 Sep 18, 2020

smehringer deleted the alignment_configuration_free_end_gaps_4 branch September 24, 2020 13:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Alignment Configuration] Adapt alignment_configurator to use the free end gap configuration from method_global #2067

[Alignment Configuration] Adapt alignment_configurator to use the free end gap configuration from method_global #2067

smehringer commented Aug 20, 2020

marehr left a comment

marehr Aug 25, 2020

smehringer Aug 25, 2020

smehringer commented Aug 27, 2020 •

edited

Loading

marehr commented Aug 27, 2020 •

edited

Loading

marehr commented Aug 27, 2020

marehr left a comment

smehringer commented Aug 27, 2020

smehringer commented Aug 28, 2020

codecov bot commented Sep 7, 2020 •

edited

Loading

smehringer commented Sep 7, 2020

eseiler commented Sep 7, 2020 •

edited

Loading

smehringer commented Sep 10, 2020 •

edited

Loading

marehr commented Sep 15, 2020 •

edited

Loading

smehringer commented Sep 15, 2020

marehr left a comment •

edited

Loading

smehringer commented Sep 15, 2020 •

edited by eseiler

Loading

rrahn left a comment

smehringer commented Sep 18, 2020

[Alignment Configuration] Adapt alignment_configurator to use the free end gap configuration from method_global #2067

[Alignment Configuration] Adapt alignment_configurator to use the free end gap configuration from method_global #2067

Conversation

smehringer commented Aug 20, 2020

marehr left a comment

Choose a reason for hiding this comment

marehr Aug 25, 2020

Choose a reason for hiding this comment

smehringer Aug 25, 2020

Choose a reason for hiding this comment

smehringer commented Aug 27, 2020 • edited Loading

marehr commented Aug 27, 2020 • edited Loading

marehr commented Aug 27, 2020

marehr left a comment

Choose a reason for hiding this comment

smehringer commented Aug 27, 2020

smehringer commented Aug 28, 2020

codecov bot commented Sep 7, 2020 • edited Loading

Codecov Report

smehringer commented Sep 7, 2020

eseiler commented Sep 7, 2020 • edited Loading

smehringer commented Sep 10, 2020 • edited Loading

marehr commented Sep 15, 2020 • edited Loading

smehringer commented Sep 15, 2020

marehr left a comment • edited Loading

Choose a reason for hiding this comment

smehringer commented Sep 15, 2020 • edited by eseiler Loading

rrahn left a comment

Choose a reason for hiding this comment

smehringer commented Sep 18, 2020

smehringer commented Aug 27, 2020 •

edited

Loading

marehr commented Aug 27, 2020 •

edited

Loading

codecov bot commented Sep 7, 2020 •

edited

Loading

eseiler commented Sep 7, 2020 •

edited

Loading

smehringer commented Sep 10, 2020 •

edited

Loading

marehr commented Sep 15, 2020 •

edited

Loading

marehr left a comment •

edited

Loading

smehringer commented Sep 15, 2020 •

edited by eseiler

Loading