[FEATURE] Switch methods to an enum vector and write own validator #64

Irallia · 2021-02-15T13:56:50Z

Resolves #59
and changes the method input parameter to an enum with an associated custom validator.

Was blocked by seqan/seqan3#2381
Was blocked by seqan/seqan3#2394

src/detect_breakends.cpp

include/method_enums.hpp

src/detect_breakends.cpp

include/method_enums.hpp

eseiler

I replied in the comment thread

eseiler

LGTM,

you can think about using enums instead of uint8_t throughout the code for the methods

src/detect_breakends/junction_detection.cpp

include/detect_breakends/junction_detection.hpp

include/method_enums.hpp

eldariont

Looks good to me. I have only a few small comments.
However, we should probably wait for seqan/seqan3#2394.

eseiler · 2021-03-02T13:40:43Z

src/detect_breakends.cpp

+    // Check method selection uses not more than all methods.
+    if (args.methods.size() > detecting_methods::SIZE)
+    {
+        seqan3::debug_stream << "[Error] More methods were selected than exist. There may be duplications.\n";


Suggested change

// Check method selection uses not more than all methods.

if (args.methods.size() > detecting_methods::SIZE)

{

seqan3::debug_stream << "[Error] More methods were selected than exist. There may be duplications.\n";

// Check that method selection contains no duplicates.

std::vector<detecting_methods> unique_methods{args.methods};

std::ranges::sort(unique_methods);

unique_methods.erase(std::unique(unique_methods.begin(), unique_methods.end()), unique_methods.end());

if (args.methods.size() > unique_methods.size())

{

seqan3::debug_stream << "[Error] The same detection method was selected multiple times.\n";

Otherwise, you can have duplications as long as you do not select more than 4.

thanks you are of course right, that was lazy programming 😇

Irallia · 2021-03-06T16:03:43Z

@eldariont After a rebase, I saw that I missed some of your review comments. Now it should be fine.
I found another bug, for which I opened an issue: #78

\param parameter_name - parameter description

And for the parameter description, I will open another PR.

eldariont · 2021-03-09T11:29:45Z

test/cli/detect_breakends_cli_test.cpp

+    "          Choose the detection method(s) to be used. Default:\n"
+    "          [cigar_string,split_read,read_pairs,read_depth]. Value must be one\n"
+    "          of\n"
+    "          [read_depth,read_depth,read_pairs,read_pairs,split_read,split_read,cigar_string,cigar_string].\n"


Why is each method included twice?

We don't use the new Validator for the detection methods enum, because I got a bug in a test. I opened an issue which should solve this problem: #78.

eldariont

Thanks for addressing my comments. In my opinion, this PR can be merged.

test/api/junction_detection_methods_test.cpp

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

Irallia self-assigned this Feb 15, 2021

Irallia force-pushed the FIX/method_selection_parameter_bugfix branch from 29ec950 to 51a4ce0 Compare February 16, 2021 13:58

Irallia marked this pull request as ready for review February 17, 2021 16:07

Irallia requested a review from eseiler February 17, 2021 16:09

eseiler requested changes Feb 17, 2021

View reviewed changes

Irallia requested a review from eseiler February 18, 2021 13:51

eseiler requested changes Feb 18, 2021

View reviewed changes

include/method_enums.hpp Outdated Show resolved Hide resolved

include/method_enums.hpp Outdated Show resolved Hide resolved

Irallia requested a review from eseiler February 18, 2021 14:41

eseiler requested changes Feb 18, 2021

View reviewed changes

eseiler approved these changes Feb 18, 2021

View reviewed changes

src/detect_breakends/junction_detection.cpp Outdated Show resolved Hide resolved

Irallia force-pushed the FIX/method_selection_parameter_bugfix branch 2 times, most recently from 86460ae to 82fc4eb Compare February 18, 2021 15:33

Irallia requested a review from eldariont February 18, 2021 15:36

Irallia force-pushed the FIX/method_selection_parameter_bugfix branch from 82fc4eb to e3a9138 Compare February 18, 2021 15:40

eldariont reviewed Feb 19, 2021

View reviewed changes

Irallia force-pushed the FIX/method_selection_parameter_bugfix branch 4 times, most recently from b932872 to df8990f Compare March 2, 2021 12:48

eseiler reviewed Mar 2, 2021

View reviewed changes

Irallia force-pushed the FIX/method_selection_parameter_bugfix branch from 519dffe to e693900 Compare March 2, 2021 16:23

Irallia requested a review from eldariont March 2, 2021 16:57

Irallia force-pushed the FIX/method_selection_parameter_bugfix branch 3 times, most recently from bc239af to fbbeb03 Compare March 6, 2021 15:59

eldariont reviewed Mar 9, 2021

View reviewed changes

eldariont approved these changes Mar 9, 2021

View reviewed changes

test/api/junction_detection_methods_test.cpp Show resolved Hide resolved

[MISC] Update SeqAn3 Version for clustering_method_validator

046fa14

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

Irallia added 2 commits March 9, 2021 16:10

[FEATURE] Switch methods to an enum vector; add a test

0a1ed51

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

[TEST] Add small check for method selection.

7edf791

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

Irallia force-pushed the FIX/method_selection_parameter_bugfix branch from fbbeb03 to 1791f7a Compare March 9, 2021 15:26

[MISC] Renaming detecting_methods -> detection_methods

616f42b

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

Irallia force-pushed the FIX/method_selection_parameter_bugfix branch from 1791f7a to 616f42b Compare March 9, 2021 18:14

Irallia merged commit d6682ff into seqan:master Mar 9, 2021

Irallia mentioned this pull request Mar 11, 2021

[TEST] Add mini example plus tests #47

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Switch methods to an enum vector and write own validator #64

[FEATURE] Switch methods to an enum vector and write own validator #64

Irallia commented Feb 15, 2021 •

edited

Loading

eseiler left a comment

eseiler left a comment

eldariont left a comment

eseiler Mar 2, 2021

Irallia Mar 2, 2021 •

edited

Loading

Irallia commented Mar 6, 2021

eldariont Mar 9, 2021

Irallia Mar 9, 2021

eldariont left a comment

-    // Check method selection uses not more than all methods.
-    if (args.methods.size() > detecting_methods::SIZE)
-    {
-        seqan3::debug_stream << "[Error] More methods were selected than exist. There may be duplications.\n";
+    // Check that method selection contains no duplicates.
+    std::vector<detecting_methods> unique_methods{args.methods};
+    std::ranges::sort(unique_methods);
+    unique_methods.erase(std::unique(unique_methods.begin(), unique_methods.end()), unique_methods.end());
+    if (args.methods.size() > unique_methods.size())
+    {
+        seqan3::debug_stream << "[Error] The same detection method was selected multiple times.\n";

[FEATURE] Switch methods to an enum vector and write own validator #64

[FEATURE] Switch methods to an enum vector and write own validator #64

Conversation

Irallia commented Feb 15, 2021 • edited Loading

eseiler left a comment

Choose a reason for hiding this comment

eseiler left a comment

Choose a reason for hiding this comment

eldariont left a comment

Choose a reason for hiding this comment

eseiler Mar 2, 2021

Choose a reason for hiding this comment

Irallia Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Irallia commented Mar 6, 2021

eldariont Mar 9, 2021

Choose a reason for hiding this comment

Irallia Mar 9, 2021

Choose a reason for hiding this comment

eldariont left a comment

Choose a reason for hiding this comment

Irallia commented Feb 15, 2021 •

edited

Loading

Irallia Mar 2, 2021 •

edited

Loading