feat: better error message when barcode length differs from expected #44

TimD1 · 2024-07-09T16:25:12Z

fixes Better error when the sample barcode lengths in the meta do not match the barcode lengths in the read structure #31, error message is now more clear

msto · 2024-07-09T17:01:16Z

src/lib/barcode_matching.rs

@@ -74,7 +74,7 @@ impl BarcodeMatcher {
        assert_eq!(
            observed_bases.len(),
            expected_bases.len(),
-            "observed_bases: {}, expected_bases: {}",
+            "Sample barcode length ({}) differs from expected ({}) during barcode matching",


Would it be helpful to additionally include the sequence of the barcode in question?

I think we want to know which sample this is caused by, as well as the barcode bases (actual and expected) along with the length.

Sounds good. I can add that info if I replace BarcodeMatcher.sample_barcodes with BarcodeMatcher.samples (vector of Sample structs storing sample_ids and barcodes). It looks like this was the eventual plan anyways:

fqtk/src/lib/barcode_matching.rs

Lines 25 to 27 in 3392552

/// Note - this is to be replaced by a sample struct in task 3. For now we're keeping things

/// very simple.

sample_barcodes: Vec<BString>,

- barcode bases and lengths (actual and expected) and sample name are printed - fixes #31 by passing `Sample` to `count_mismatches()` instead of barcode string - all `count_mismatches()` tests updated to match new signature - minor miscellaneous comment typos fixed as well

TimD1 · 2024-07-10T21:18:06Z

This is my first time writing Rust, so please feel free to make suggestions for better style/design/etc.

A note on the new tests:
Since Samples do not allow empty barcodes:

fqtk/src/lib/samples.rs

Lines 48 to 50 in 6ed98a3

    
           pub fn new(ordinal: usize, name: String, barcode: String) -> Self { 
        
               assert!(!name.is_empty(), "Sample name cannot be empty"); 
        
               assert!(!barcode.is_empty(), "Sample barcode cannot be empty");

My understanding is that new() is the only way to get a new Sample outside of the samples.rs module, since Sample contains a private field (ordinal). Therefore, I dropped the test for BarcodeMatcher being initialized with a Sample with an empty barcode:

fqtk/src/lib/barcode_matching.rs

Lines 178 to 185 in 6ed98a3

    
           #[rstest] 
        
           #[case(true)] 
        
           #[case(false)] 
        
           #[should_panic(expected = "Sample barcode cannot be empty string")] 
        
           fn test_barcode_matcher_fails_if_empty_sample_barcode_provided(#[case] use_cache: bool) { 
        
               let sample_barcodes = vec!["AGCT", ""]; 
        
               let _matcher = BarcodeMatcher::new(&sample_barcodes, 2, 1, use_cache); 
        
           }

And modified the test that allowed empty barcodes to be compared to check that it now fails.

fqtk/src/lib/barcode_matching.rs

Lines 195 to 198 in 6ed98a3

    
           #[test] 
        
           fn empty_string_can_run_in_count_mismatches() { 
        
               assert_eq!(BarcodeMatcher::count_mismatches("".as_bytes(), "".as_bytes()), 0); 
        
           }

msto reviewed Jul 9, 2024

View reviewed changes

TimD1 force-pushed the 31/td/barcode-length-mismatch-error-message branch from 3392552 to 745cdb8 Compare July 10, 2024 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: better error message when barcode length differs from expected #44

feat: better error message when barcode length differs from expected #44

TimD1 commented Jul 9, 2024

msto Jul 9, 2024

nh13 Jul 9, 2024

TimD1 Jul 9, 2024

TimD1 commented Jul 10, 2024

	/// Note - this is to be replaced by a sample struct in task 3. For now we're keeping things
	/// very simple.
	sample_barcodes: Vec<BString>,

feat: better error message when barcode length differs from expected #44

Are you sure you want to change the base?

feat: better error message when barcode length differs from expected #44

Conversation

TimD1 commented Jul 9, 2024

msto Jul 9, 2024

Choose a reason for hiding this comment

nh13 Jul 9, 2024

Choose a reason for hiding this comment

TimD1 Jul 9, 2024

Choose a reason for hiding this comment

TimD1 commented Jul 10, 2024