[FIX] Use signed integer types for CLI parameters

- change args.min_var_length from uint32_t to int32_t
- change args.max_var_length from uint32_t to int32_t
- change args.max_tol_inserted_length from uint32_t to int32_t
- change args.max_overlap from uint32_t to int32_t
- expand CLI documentation (noting that params need non-negative value)
- expand documentation of all functions accepting these arguments
 (noting that it expects a non-negative value)
 - explicitly check for non-negative value and terminate if negative
 - add test for this check with negative parameter
 - replace abs with std::abs in analyze_sa_tag_method.cpp

Reason:
When comparing signed with unsigned values
(e.g. int32 and uint32), C++ implicitly converts the signed
type into an unsigned type which leads to unexpected
behavior (e.g. -1 gets converted to a large positive value).

As a solution, we should not use unsigned types for quantities
on which we might want to perform mathematical operations
or comparisons. To make sure that the parameters do not take
negative values we add an explicit check that terminates the
program if the user gave a negative value.

Author: David Heller

include/iGenVar.hpp

@@ -12,10 +12,10 @@ struct cmd_arguments
     std::vector<detection_methods> methods{cigar_string, split_read, read_pairs, read_depth};   // default: all methods
     clustering_methods clustering_method{hierarchical_clustering};                              // default: hierarchical clustering method
     refinement_methods refinement_method{no_refinement};                                        // default: no refinement
-    uint32_t min_var_length = 30;
-    uint32_t max_var_length = 1000000;
-    uint32_t max_tol_inserted_length = 5;
-    uint32_t max_overlap = 10;
+    int32_t min_var_length = 30;
+    int32_t max_var_length = 1000000;
+    int32_t max_tol_inserted_length = 5;
+    int32_t max_overlap = 10;
 };
 
 void initialize_argument_parser(seqan3::argument_parser & parser, cmd_arguments & args);
@@ -38,10 +38,10 @@ void initialize_argument_parser(seqan3::argument_parser & parser, cmd_arguments
  *                      (0: no_refinement,
  *                       1: sViper_refinement_method,
  *                       2: sVirl_refinement_method) - *default: no refinement*\n
- *                   **args.min_var_length** - minimum length of variants to detect - *default: 30 bp*\n
- *                   **args.max_var_length** - maximum length of variants to detect - *default: 1,000,000 bp*\n
- *                   **args.max_tol_inserted_length** - longest tolerated inserted sequence at non-INS SV types - *default: 5 bp*\n
- *                   **args.max_overlap** - maximum overlap between alignment segments - *default: 10 bp*
+ *                   **args.min_var_length** - minimum length of variants to detect (expected to be non-negative) - *default: 30 bp*\n
+ *                   **args.max_var_length** - maximum length of variants to detect (expected to be non-negative) - *default: 1,000,000 bp*\n
+ *                   **args.max_tol_inserted_length** - longest tolerated inserted sequence at non-INS SV types (expected to be non-negative) - *default: 5 bp*\n
+ *                   **args.max_overlap** - maximum overlap between alignment segments (expected to be non-negative) - *default: 10 bp*
  *
  *
  * \details Detects novel junctions from read alignment records using different detection methods.

include/modules/sv_detection_methods/analyze_cigar_method.hpp

@@ -10,7 +10,7 @@
  * \param[in]       cigar_string    - CIGAR field of the SAM/BAM file
  * \param[in]       query_sequence  - SEQ field of the SAM/BAM file
  * \param[in, out]  junctions       - vector for storing junctions
- * \param[in]       min_length      - minimum length of variants to detect (default 30 bp)
+ * \param[in]       min_length      - minimum length of variants to detect (default 30 bp, expected to be non-negative)
  *
  * \details This function steps through the CIGAR string and stores junctions with their position in reference and read.
  *          We distinguish 4 cases of CIGAR operation characters:
@@ -36,4 +36,4 @@ void analyze_cigar(std::string const & read_name,
                    std::vector<seqan3::cigar> & cigar_string,
                    seqan3::dna5_vector const & query_sequence,
                    std::vector<Junction> & junctions,
-                   uint32_t const min_length);
+                   int32_t const min_length);

include/modules/sv_detection_methods/analyze_sa_tag_method.hpp

@@ -42,15 +42,15 @@ void retrieve_aligned_segments(std::string const & sa_string, std::vector<Aligne
  * \param[in, out]  junctions           - vector for storing junctions
  * \param[in, out]  query_sequence      - SEQ field of the SAM/BAM file
  * \param[in]       read_name           - QNAME field of the SAM/BAM file
- * \param[in]       min_length          - minimum length of variants to detect
- * \param[in]       max_overlap         - maximum overlap between alignment segments
+ * \param[in]       min_length          - minimum length of variants to detect (expected to be non-negative)
+ * \param[in]       max_overlap         - maximum overlap between alignment segments (expected to be non-negative)
  */
 void analyze_aligned_segments(std::vector<AlignedSegment> const & aligned_segments,
                               std::vector<Junction> & junctions,
                               seqan3::dna5_vector const & query_sequence,
                               std::string const & read_name,
-                              uint32_t const min_length,
-                              uint32_t const max_overlap);
+                              int32_t const min_length,
+                              int32_t const max_overlap);
 
 /*! \brief Parse the SA tag from the SAM/BAM alignment of a chimeric/split-aligned read. Build
  *         [aligned_segments](\ref AlignedSegment), one for each alignment segment of the read.
@@ -65,8 +65,8 @@ void analyze_aligned_segments(std::vector<AlignedSegment> const & aligned_segmen
  * \param[in]       seq         - SEQ field of the SAM/BAM file
  * \param[in]       sa_tag      - SA tag, one tag from the read of the SAM/BAM file
  * \param[in]       args        - command line arguments:\n
- *                                **args.min_var_length** - minimum length of variants to detect\n
- *                                **args.max_overlap** - maximum overlap between alignment segments
+ *                                **args.min_var_length** - minimum length of variants to detect (expected to be non-negative)\n
+ *                                **args.max_overlap** - maximum overlap between alignment segments (expected to be non-negative)
  * \param[in, out]  junctions   - vector for storing junctions
  */
 void analyze_sa_tag(std::string const & query_name,

include/variant_detection/variant_detection.hpp

@@ -32,8 +32,8 @@ void detect_junctions_in_short_reads_sam_file(std::vector<Junction> & junctions,
  *                         **args.alignment_long_reads_file_path** - long reads input file, path to the sam/bam file\n
  *                         **args.methods** - list of methods for detecting junctions
  *                            (0: cigar_string, 1: split_read, 2: read_pairs, 3: read_depth) - *default: all methods*\n
- *                         **args.min_var_length** - minimum length of variants to detect - *default: 30 bp*\n
- *                         **args.max_overlap** - maximum overlap between alignment segments - *default: 10 bp*
+ *                         **args.min_var_length** - minimum length of variants to detect (expected to be non-negative) - *default: 30 bp*\n
+ *                         **args.max_overlap** - maximum overlap between alignment segments (expected to be non-negative) - *default: 10 bp*
  *
  *
  * \details Detects junctions from the CIGAR strings and supplementary alignment tags of read alignment records.

include/variant_detection/variant_output.hpp

@@ -12,9 +12,9 @@
  *
  * \param[in]       clusters    - input junction clusters
  * \param[in]       args        - command line arguments:\n
- *                                **args.min_var_length** - minimum length of variants to detect - *default: 30 bp*\n
- *                                **args.max_var_length** - maximum length of variants to detect - *default: 1,000,000 bp*\n
- *                                **args.max_tol_inserted_length** - longest tolerated inserted sequence at non-INS SV types - *default: 5 bp*
+ *                                **args.min_var_length** - minimum length of variants to detect (expected to be non-negative) - *default: 30 bp* \n
+ *                                **args.max_var_length** - maximum length of variants to detect (expected to be non-negative) - *default: 1,000,000 bp*\n
+ *                                **args.max_tol_inserted_length** - longest tolerated inserted sequence at non-INS SV types (expected to be non-negative) - *default: 5 bp*
  * \param[in, out]  out_stream  - output stream
  *
  * \details Extracts genomic variants from given junction clusters.
@@ -29,9 +29,9 @@ void find_and_output_variants(std::vector<Cluster> const & clusters,
  *
  * \param[in] clusters          - input junction clusters
  * \param[in] args              - command line arguments:\n
- *                                **args.min_var_length** - minimum length of variants to detect - *default: 30 bp*\n
- *                                **args.max_var_length** - maximum length of variants to detect - *default: 1,000,000 bp*\n
- *                                **args.max_tol_inserted_length** - longest tolerated inserted sequence at non-INS SV types - *default: 5 bp*
+ *                                **args.min_var_length** - minimum length of variants to detect (expected to be non-negative) - *default: 30 bp*\n
+ *                                **args.max_var_length** - maximum length of variants to detect (expected to be non-negative) - *default: 1,000,000 bp*\n
+ *                                **args.max_tol_inserted_length** - longest tolerated inserted sequence at non-INS SV types (expected to be non-negative) - *default: 5 bp*
  * \param[in] output_file_path  - output file path
  *
  * \details Extracts genomic variants from given junction clusters.

src/iGenVar.cpp

@@ -56,17 +56,21 @@ void initialize_argument_parser(seqan3::argument_parser & parser, cmd_arguments
 
     // Options - SV specifications:
     parser.add_option(args.min_var_length, 'l', "min_var_length",
-                      "Specify what should be the minimum length of your SVs to be detected.",
+                      "Specify what should be the minimum length of your SVs to be detected. "
+                      "This value needs to be non-negative.",
                       seqan3::option_spec::advanced);
     parser.add_option(args.max_var_length, 'x', "max_var_length",
                       "Specify what should be the maximum length of your SVs to be detected. "
-                      "SVs larger than this threshold can still be output as translocations.",
+                      "SVs larger than this threshold can still be output as translocations. "
+                      "This value needs to be non-negative.",
                       seqan3::option_spec::advanced);
     parser.add_option(args.max_tol_inserted_length, 't', "max_tol_inserted_length",
-                      "Specify what should be the longest tolerated inserted sequence at sites of non-INS SVs.",
+                      "Specify what should be the longest tolerated inserted sequence at sites of non-INS SVs. "
+                      "This value needs to be non-negative.",
                       seqan3::option_spec::advanced);
     parser.add_option(args.max_overlap, 'p', "max_overlap",
-                      "Specify the maximum allowed overlap between two alignment segments.",
+                      "Specify the maximum allowed overlap between two alignment segments. "
+                      "This value needs to be non-negative.",
                       seqan3::option_spec::advanced);
 }
 
@@ -167,6 +171,28 @@ int main(int argc, char ** argv)
         return -1;
     }
 
+    // Check that the given parameters are non-negative.
+    if (args.min_var_length < 0)
+    {
+        seqan3::debug_stream << "[Error] You gave a negative min_var_length parameter.\n";
+        return -1;
+    }
+    if (args.max_var_length < 0)
+    {
+        seqan3::debug_stream << "[Error] You gave a negative max_var_length parameter.\n";
+        return -1;
+    }
+    if (args.max_tol_inserted_length < 0)
+    {
+        seqan3::debug_stream << "[Error] You gave a negative max_tol_inserted_length parameter.\n";
+        return -1;
+    }
+    if (args.max_overlap < 0)
+    {
+        seqan3::debug_stream << "[Error] You gave a negative max_overlap parameter.\n";
+        return -1;
+    }
+
     detect_variants_in_alignment_file(args);
 
     return 0;

src/modules/sv_detection_methods/analyze_cigar_method.cpp

@@ -15,7 +15,7 @@ void analyze_cigar(std::string const & read_name,
                    std::vector<seqan3::cigar> & cigar_string,
                    seqan3::dna5_vector const & query_sequence,
                    std::vector<Junction> & junctions,
-                   uint32_t const min_length)
+                   int32_t const min_length)
 {
     // Step through CIGAR string and store current position in reference and read
     int32_t pos_ref = query_start_pos;

src/modules/sv_detection_methods/analyze_sa_tag_method.cpp

@@ -57,8 +57,8 @@ void analyze_aligned_segments(std::vector<AlignedSegment> const & aligned_segmen
                               std::vector<Junction> & junctions,
                               seqan3::dna5_vector const & query_sequence,
                               std::string const & read_name,
-                              uint32_t const min_length,
-                              uint32_t const max_overlap)
+                              int32_t const min_length,
+                              int32_t const max_overlap)
 {
     for (size_t i = 1; i < aligned_segments.size(); i++)
     {
@@ -67,7 +67,7 @@ void analyze_aligned_segments(std::vector<AlignedSegment> const & aligned_segmen
         int32_t distance_on_read = next.get_query_start() - current.get_query_end();
         // Check that the overlap between two consecutive alignment segments
         // of the read is lower than the given threshold
-        if (distance_on_read >= -static_cast<int32_t>(max_overlap))
+        if (distance_on_read >= -max_overlap)
         {
             int32_t mate1_pos;
             if (current.orientation == strand::forward)
@@ -99,7 +99,7 @@ void analyze_aligned_segments(std::vector<AlignedSegment> const & aligned_segmen
             // have a large distance on the reference (e.g. deletion, inversion, tandem duplication), or
             // have a large distance on the read (e.g. insertion)
             if (current.ref_name != next.ref_name ||
-                abs(distance_on_ref) >= min_length ||
+                std::abs(distance_on_ref) >= min_length ||
                 distance_on_read >= min_length)
             {
                 Breakend mate1{current.ref_name,

test/api/detection_test.cpp

@@ -27,7 +27,7 @@ TEST(junction_detection, cigar_string_simple_del)
     // Deletion smaller than minimum variant size
     {
         std::vector<Junction> junctions_res{};
-        uint64_t const min_var_length = 10;
+        int32_t const min_var_length = 10;
         analyze_cigar(read_name, chromosome, query_start_pos, cigar_string, seq, junctions_res, min_var_length);
 
         EXPECT_EQ(junctions_res.size(), 0);
@@ -36,7 +36,7 @@ TEST(junction_detection, cigar_string_simple_del)
     // Deletion larger than minimum variant size
     {
         std::vector<Junction> junctions_res{};
-        uint64_t const min_var_length = 5;
+        int32_t const min_var_length = 5;
         analyze_cigar(read_name, chromosome, query_start_pos, cigar_string, seq, junctions_res, min_var_length);
 
         Breakend new_breakend_1 {chromosome, 15, strand::forward};
@@ -66,7 +66,7 @@ TEST(junction_detection, cigar_string_del_padding)
     seqan3::dna5_vector seq = {"GGGCTCATCGATCGATTTCGGATCGGGGGGCCCCCATTTTAAACGGCCCC"_dna5};
 
     std::vector<Junction> junctions_res{};
-    uint64_t const min_var_length = 5;
+    int32_t const min_var_length = 5;
     analyze_cigar(read_name, chromosome, query_start_pos, cigar_string, seq, junctions_res, min_var_length);
 
     Breakend new_breakend_1 {chromosome, 15, strand::forward};
@@ -94,7 +94,7 @@ TEST(junction_detection, cigar_string_simple_ins)
     seqan3::dna5_vector seq = {"GGGCTCATCGATCGATTTCGGATCGGGGGGCCCCCATTTTAAACGGCCCC"_dna5};
 
     std::vector<Junction> junctions_res{};
-    uint64_t const min_var_length = 5;
+    int32_t const min_var_length = 5;
     analyze_cigar(read_name, chromosome, query_start_pos, cigar_string, seq, junctions_res, min_var_length);
 
     Breakend new_breakend_1 {chromosome, 9, strand::forward};
@@ -122,7 +122,7 @@ TEST(junction_detection, cigar_string_ins_hardclip)
     seqan3::dna5_vector seq = {"GGGCTCATCGATCGATTTCGGATCGGGGGGCCCCCATTTTAAACGGCCCC"_dna5};
 
     std::vector<Junction> junctions_res{};
-    uint64_t const min_var_length = 5;
+    int32_t const min_var_length = 5;
     analyze_cigar(read_name, chromosome, query_start_pos, cigar_string, seq, junctions_res, min_var_length);
 
     Breakend new_breakend_1 {chromosome, 9, strand::forward};

test/api/input_file_test.cpp

@@ -12,8 +12,8 @@ std::string const default_alignment_short_reads_file_path = DATADIR"paired_end_m
 std::string const default_alignment_long_reads_file_path = DATADIR"simulated.minimap2.hg19.coordsorted_cutoff.sam";
 std::filesystem::path const empty_output_path{};
 std::vector<detection_methods> const default_methods{cigar_string, split_read, read_pairs, read_depth};
-constexpr uint64_t default_min_length = 30;
-constexpr uint64_t default_max_overlap = 10;
+constexpr int32_t default_min_length = 30;
+constexpr int32_t default_max_overlap = 10;
 
 // Explanation for the strings:
 // chr21\t41972615\tForward\tchr21\t41972616\tForward\t1\t1681