-
Notifications
You must be signed in to change notification settings - Fork 8
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] Modularization of the methods for detecting junctions. #45
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,34 +6,60 @@ struct cmd_arguments | |
{ | ||
std::filesystem::path alignment_file_path{}; | ||
std::filesystem::path insertion_file_path{}; | ||
std::vector<uint8_t> methods{1, 2, 3, 4}; // default is using all methods | ||
}; | ||
|
||
void initialize_argument_parser(seqan3::argument_parser & parser, cmd_arguments & args) | ||
{ | ||
parser.info.author = "David Heller"; | ||
parser.info.author = "David Heller & Lydia Buntrock"; | ||
parser.info.app_name = "iGenVar"; | ||
parser.info.man_page_title = "Short and long Read SV Caller"; | ||
parser.info.short_description = "Detect junctions in a read alignment file"; | ||
parser.info.version = "0.0.1"; | ||
parser.info.date = "19-01-2021"; // last update | ||
parser.info.email = "lydia.buntrock@fu-berlin.de"; | ||
parser.info.long_copyright = "long_copyright"; | ||
parser.info.short_copyright = "short_copyright"; | ||
parser.info.url = "https://github.com/seqan/iGenVar/"; | ||
|
||
// Validatiors: | ||
// seqan3::value_list_validator method_validator{"1", "cigar_string", | ||
// "2", "split_read", | ||
// "3", "read_pairs", | ||
// "4", "read_depth"}; | ||
Comment on lines
+26
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking, if the user wants to insert names instead of numbers, so we could offer both. |
||
seqan3::arithmetic_range_validator method_validator{1, 4}; | ||
|
||
// Options - Input / Output: | ||
parser.add_positional_option(args.alignment_file_path, "Input read alignments in SAM or BAM format.", | ||
seqan3::input_file_validator{{"sam", "bam"}} ); | ||
parser.add_positional_option(args.insertion_file_path, "Output file for insertion alleles", | ||
seqan3::output_file_validator{{"fa", "fasta"}} ); | ||
|
||
// Options - Methods: | ||
parser.add_option(args.methods, 'm', "method", "Choose the method to be used.", | ||
seqan3::option_spec::ADVANCED, method_validator); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way this is, a user could only choose one of the four methods, no? E.g. you can't use Although to be completely honest, I can't really think of a reason why a user would not want to use the CIGAR string. For split reads, if the user has very short reads I guess they might think split reads aren't useful. Same for read depth if the sequencing was very shallow. With regards to read pairs, I think this is something we should detect automatically (whether the sequencing was done with single end or paired end sequencing), so that a user doesn't accidentally turn on the read pairs option with single end sequencing data. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think with the current implementation, a user can choose four methods using something like You have a good point, though, with the CIGAR strings and the read pairs. It might be good to spend some time thinking about efficient processing of the SAM/BAM files:
So we should maybe have a separate detection function for read depth because it's completely separate from the other methods. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think my change (the modularisation) is there to compare all methods with all methods. |
||
} | ||
|
||
int main(int argc, char ** argv) | ||
{ | ||
seqan3::argument_parser myparser{"detectJunctions", argc, argv}; // initialise myparser | ||
cmd_arguments args{}; | ||
initialize_argument_parser(myparser, args); | ||
|
||
// Parse the given arguments and catch possible errors. | ||
try | ||
{ | ||
myparser.parse(); // trigger command line parsing | ||
} | ||
catch (seqan3::argument_parser_error const & ext) // catch user errors | ||
{ | ||
seqan3::debug_stream << "[Error] " << ext.what() << "\n"; // customise your error message | ||
seqan3::debug_stream << "[Error] " << ext.what() << '\n'; // customise your error message | ||
return -1; | ||
} | ||
detect_junctions_in_alignment_file(args.alignment_file_path, args.insertion_file_path); | ||
|
||
detect_junctions_in_alignment_file(args.alignment_file_path, | ||
args.insertion_file_path, | ||
args.methods); | ||
|
||
return 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this function implements all four detection methods but can be parametrized by the
methods
parameter to only use a subset of methods. This makes a lot of sense because the alignment file has to be processed only once (in the best case) and different methods can be applied on the same read in one go.However, this architecture is not really modularized, right? To be really modularized we would have four
detect_junctions
functions that each implement one method. But then, the alignment file would be processed four times in the worst case. So I guess that's why you decided to choose the other option?!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is correct. Maybe the term modularize is not really the right one.
The idea is, that we can combine different methods. And compare them. As we also want to compare run time, the complete decoupling would end in a higher run time if we combine two methods.
So I would leave it and would change the term modularisation with decoupling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. But I think it's not really a decoupling either. How about "Enable selection of methods for detecting junctions" as new title for this pull request?