Client-side specimen ID matching

labcontrol/db/process.py

@@ -296,10 +296,9 @@ def update_well(self, row, col, content):
 
         Returns
         -------
-        str
-            The new contents of the well
-        bool
-            Whether or not the new contents of the well are "ok"
+        (str, bool)
+            The str corresponds to the new contents of the well; the bool
+            indicates whether or not the new contents of the well are "ok"
         """
         return self.plate.get_well(row, col).composition.update(content)
 

labcontrol/db/study.py

@@ -190,8 +190,8 @@ def samples(self, term=None, limit=None):
         ----------
         term: str, optional
             If provided, return only the samples that contain the given term
-        limit: int, optional
-            If provided, don't return more than `limit` results
+        limit: str, optional
+            If provided, don't return more than `int(limit)` results
 
         Returns
         -------
@@ -202,7 +202,8 @@ def samples(self, term=None, limit=None):
         Raises
         ------
         ValueError
-            If `type(limit) != int`, or if `limit` is less than or equal to 0.
+            If `int(limit)` raises a ValueError or OverflowError, or if
+            `limit` is less than or equal to 0.
         """
 
         # SQL generation moved outside of the with conditional to enhance
@@ -228,10 +229,18 @@ def samples(self, term=None, limit=None):
             order_by_clause = "order by sample_values->'%s'" % column
 
         if limit is not None:
-            if type(limit) != int:
-                raise ValueError("limit must be an int")
+            # Attempt to cast the limit to an int, and (if that works) verify
+            # that the integer limit is greater than zero
+            try:
+                limit = int(limit)
+            except (ValueError, OverflowError, TypeError):
+                # Examples of why we catch these particular exceptions:
+                # - ValueError is most "common," occurs with int("abc")
+                # - OverflowError occurs with int(float("inf"))
+                # - TypeError occurs with int([1,2,3])
+                raise ValueError("limit must be castable to an int")
             if limit <= 0:
-                raise ValueError("limit can't be <= 0")
+                raise ValueError("limit must be greater than zero")
             order_by_clause += " limit %d" % limit
 
         if column == 'sample_id':

labcontrol/db/tests/test_study.py

@@ -6,6 +6,7 @@
 # The full license is in the file LICENSE, distributed with this software.
 # ----------------------------------------------------------------------------
 
+from math import floor
 from unittest import main
 
 from labcontrol.db.testing import LabControlTestCase
@@ -65,13 +66,13 @@ def test_samples_with_sample_id(self):
         exp_samples = ['1.SKB1.640202', '1.SKB2.640194', '1.SKB3.640195',
                        '1.SKB4.640189', '1.SKB5.640181', '1.SKB6.640176',
                        '1.SKB7.640196', '1.SKB8.640193', '1.SKB9.640200']
-        self.assertEqual(s.samples(limit=9), exp_samples)
+        self.assertEqual(s.samples(limit='9'), exp_samples)
         exp_samples = ['1.SKB1.640202', '1.SKB2.640194', '1.SKB3.640195',
                        '1.SKB4.640189', '1.SKB5.640181', '1.SKB6.640176',
                        '1.SKB7.640196', '1.SKB8.640193', '1.SKB9.640200']
         self.assertEqual(s.samples('SKB'), exp_samples)
         exp_samples = ['1.SKB1.640202', '1.SKB2.640194', '1.SKB3.640195']
-        self.assertEqual(s.samples('SKB', limit=3), exp_samples)
+        self.assertEqual(s.samples('SKB', limit='3'), exp_samples)
         exp_samples = ['1.SKM1.640183', '1.SKM2.640199', '1.SKM3.640197',
                        '1.SKM4.640180', '1.SKM5.640177', '1.SKM6.640187',
                        '1.SKM7.640188', '1.SKM8.640201', '1.SKM9.640192']
@@ -84,6 +85,17 @@ def test_samples_with_sample_id(self):
         self.assertEqual(s.samples('1.64'), exp_samples)
 
     def test_samples_with_limit(self):
+        """Unit-tests the `limit` argument of Study.samples() in particular.
+
+        It's worth noting that the `limit` value that StudySamplesHandler.get()
+        uses when calling Study.samples() is actually a string -- this is due
+        to our use of tornado.web.RequestHandler.get_argument().
+        Study.samples() only cares that `int(limit)` succeeds, and is otherwise
+        agnostic to the actual input type of `limit`.
+
+        (For the sake of caution, we test a couple of types besides purely
+        `str` values within this function.)
+        """
         s = Study(1)
         all_samples = ['1.SKB1.640202', '1.SKB2.640194', '1.SKB3.640195',
                        '1.SKB4.640189', '1.SKB5.640181', '1.SKB6.640176',
@@ -97,31 +109,60 @@ def test_samples_with_limit(self):
         # Check cases where the limit is valid but doesn't actually result in
         # any filtering being done.
         self.assertEqual(s.samples(), all_samples)
-        self.assertEqual(s.samples(limit=None), all_samples)
-        self.assertEqual(s.samples(limit=27), all_samples)
-        self.assertEqual(s.samples(limit=28), all_samples)
-        self.assertEqual(s.samples(limit=50), all_samples)
-        self.assertEqual(s.samples(limit=100), all_samples)
-        self.assertEqual(s.samples(limit=10000), all_samples)
-        # limit=None is the default, but we check it here explicitly anyway
+        for i in [27, 28, 50, 100, 10000]:
+            self.assertEqual(s.samples(limit=i), all_samples)
+            self.assertEqual(s.samples(limit=str(i)), all_samples)
+        # limit=None is the default, but we check it here explicitly anyway.
         self.assertEqual(s.samples(limit=None), all_samples)
 
-        # Check *all* limit values in the inclusive range [1, 27]
+        # Check *all* limit values in the inclusive range [1, 27] -- these
+        # should, well, limit the output list of samples accordingly
         for i in range(1, len(all_samples)):
             self.assertEqual(s.samples(limit=i), all_samples[:i])
+            self.assertEqual(s.samples(limit=str(i)), all_samples[:i])
 
-        # Check that non-int limits cause an error
-        nonint_limits_to_test = [0.01, 0.5, 1.0, 1.2, 3.0, 27.0, 29.1, 1000.0]
-        nonint_limits_to_test += [[1, 2, 3], "abc", "123", "1"]
-        for i in nonint_limits_to_test:
-            with self.assertRaisesRegex(ValueError, "limit must be an int"):
+        float_limits_to_test = [1.0, 1.2, 3.0, 27.0, 29.1, 1000.0]
+        str_of_float_limits_to_test = [str(f) for f in float_limits_to_test]
+
+        # Test that various not-castable-to-a-base-10-int inputs don't work
+        # (This includes string representations of floats, e.g. "1.0", since
+        # such a string is not a valid "integer literal" -- see
+        # https://docs.python.org/3/library/functions.html#int.
+        uncastable_limits_to_test = [
+            [1, 2, 3], "abc", "gibberish", "ten", (1,), "0xBEEF", "0b10101",
+            "0o123", float("inf"), float("-inf"), float("nan"), "None", "inf"
+        ]
+        for i in uncastable_limits_to_test + str_of_float_limits_to_test:
+            with self.assertRaisesRegex(
+                ValueError, "limit must be castable to an int"
+            ):
                 s.samples(limit=i)
 
+        # Calling int(x) where x is a float just truncates x "towards zero"
+        # according to https://docs.python.org/3/library/functions.html#int.
+        #
+        # This behavior is tested, but it should never happen (one, because
+        # as of writing Study.samples() is only called with a string limit
+        # value, and two because I can't imagine why someone would pass a float
+        # in for the "limit" argument).
+        for i in float_limits_to_test:
+            self.assertEqual(s.samples(limit=i), all_samples[:floor(i)])
+
         # Check that limits <= 0 cause an error
-        for i in range(0, -len(all_samples), -1):
-            with self.assertRaisesRegex(ValueError, "limit can't be <= 0"):
+        nonpositive_limits = [0, -1, -2, -27, -53, -100]
+        for i in nonpositive_limits:
+            with self.assertRaisesRegex(
+                ValueError, "limit must be greater than zero"
+            ):
                 s.samples(limit=i)
 
+        # Check evil corner case where the limit is nonpositive and not
+        # castable to an int (this should fail first on the castable check)
+        with self.assertRaisesRegex(
+            ValueError, "limit must be castable to an int"
+        ):
+            s.samples(limit="-1.0")
+
     def test_samples_with_specimen_id(self):
         s = Study(1)
 

labcontrol/gui/handlers/study.py

@@ -50,17 +50,10 @@ def get(self, study_id):
         try:
             study = Study(int(study_id))
             term = self.get_argument('term', None)
-
-            # Default limit is None (i.e. give back all samples)
             limit = self.get_argument('limit', None)
-            if limit is not None:
-                # If this fails, it'll result in a ValueError
-                limit = int(limit)
-
-            # If limit is somehow invalid (<= 0 or not an int), study.samples()
-            # will raise a ValueError. The interpretation is the same as with
-            # the possible ValueError raised on int(limit) -- the limit is
-            # invalid, so we'll set a 400 ("Bad Request") status code.
+            # If the specified limit is somehow invalid, study.samples() will
+            # raise a ValueError. In this case, we'll set a 400 ("Bad Request")
+            # status code.
             res = list(study.samples(term, limit))
             self.write(json_encode(res))
         except LabControlUnknownIdError:

labcontrol/gui/js_tests/labcontrol_tests.js

@@ -82,16 +82,16 @@ QUnit.module("clippingForPlateType", function(hooks) {
 
 QUnit.module("getSubstringMatches", function(hooks) {
   QUnit.test("common usage", function(assert) {
-    assert.propEqual(getSubstringMatches("w", ["wahoo", "walrus"]), [
+    assert.deepEqual(getSubstringMatches("w", ["wahoo", "walrus"]), [
       "wahoo",
       "walrus"
     ]);
-    assert.propEqual(getSubstringMatches("h", ["wahoo", "walrus"]), ["wahoo"]);
+    assert.deepEqual(getSubstringMatches("h", ["wahoo", "walrus"]), ["wahoo"]);
     assert.equal(getSubstringMatches("z", ["wahoo", "walrus"]).length, 0);
-    assert.propEqual(getSubstringMatches("1234f", ["01234f", "01234"]), [
+    assert.deepEqual(getSubstringMatches("1234f", ["01234f", "01234"]), [
       "01234f"
     ]);
-    assert.propEqual(
+    assert.deepEqual(
       getSubstringMatches("abc", ["abc", "ABCDEF", "AbCdE", "", "DEF"]),
       ["abc", "ABCDEF", "AbCdE"]
     );
@@ -102,7 +102,7 @@ QUnit.module("getSubstringMatches", function(hooks) {
     assert.equal(getSubstringMatches("", ["wahoo", "walrus"]).length, 0);
     assert.equal(getSubstringMatches("", ["wahoo", "walrus", ""]).length, 0);
     assert.equal(getSubstringMatches("abc", ["wahoo", "walrus", ""]).length, 0);
-    assert.propEqual(getSubstringMatches("w", ["wahoo", "walrus", ""]), [
+    assert.deepEqual(getSubstringMatches("w", ["wahoo", "walrus", ""]), [
       "wahoo",
       "walrus"
     ]);

labcontrol/gui/static/js/labcontrol.js

@@ -746,7 +746,8 @@ function createHeatmap(
 /**
  *
  * Given a query string and an array of strings to search through, returns a
- * subset of the array containing only strings that contain the query string.
+ * subset of the array containing only strings that contain the query string
+ * (or an empty array, if no such matches are found).
  *
  * This search is case insensitive, so "abc" will match with "ABCDEF".
  *

labcontrol/gui/static/js/plateViewer.js

@@ -434,14 +434,13 @@ PlateViewer.prototype.getActiveStudy = function() {
 
 /**
  *
- * Actually updates a well's content in the backend. This code was ripped out
+ * Actually update a well's content in the backend. This code was ripped out
  * of PlateViewer.modifyWell().
  *
  * @param {int} row The row of the well being modified
  * @param {int} col The column of the well being modified
  * @param {string} content The new content of the well
- * @param {string} studyID The output of getActiveStudy() -- this paradigm
- *      should really be changed in the future to accommodate multiple studies.
+ * @param {string} studyID The output of getActiveStudy()
  *
  */
 PlateViewer.prototype.patchWell = function(row, col, content, studyID) {
@@ -457,10 +456,6 @@ PlateViewer.prototype.patchWell = function(row, col, content, studyID) {
     },
     success: function(data) {
       that.data[row][that.grid.getColumns()[col].field] = data["sample_id"];
-      // NOTE: this pretty much checks the status of every well, and this is
-      // called in patchWell (i.e. every time a well is updated). Should be a
-      // lot more efficient to minimize the impact of this by just checking for
-      // changes relative to this well?
       that.updateUnknownsAndDuplicates();
       var classIdx = that.wellClasses[row][col].indexOf("well-prev-plated");
       if (data["previous_plates"].length > 0) {
@@ -501,13 +496,15 @@ PlateViewer.prototype.modifyWell = function(row, col, content) {
   // or even if the user selects something from a dropdown list. (That later
   // case could probably be detected in order to avoid doing matching here.)
   //
-  // I expect the actual matching to be pretty fast (assuming that there
-  // aren't thousands of active samples); the main bottleneck is how requests
-  // are made to the server from modifyWell() and patchWell() instead of in
-  // batch operations.
-  var possiblyNewContent = content;
+  // This functionality has not been stress-tested on large-scale datasets
+  // (e.g. the American Gut Project) yet; this is a major TODO, as part of
+  // issue #173 on GitHub.
+  //
+  // A major bottleneck here, I think, will be how requests are made on a well-
+  // by-well basis to the server in patchWell() instead of in batch operations.
+  var possiblyMatchedContent = content;
   // TODO: cache list of active samples so that we don't have to make this
-  // particular request every time modifyWell() is called.
+  // particular request every time modifyWell() is called. See #592 in GH repo.
   get_active_samples().then(
     function(sampleIDs) {
       // If there is *exactly one* match with an active sample ID, use
@@ -516,18 +513,20 @@ PlateViewer.prototype.modifyWell = function(row, col, content) {
       // cell content.
       var matchingSamples = getSubstringMatches(content, sampleIDs);
       if (matchingSamples.length === 1) {
-        possiblyNewContent = matchingSamples[0];
+        possiblyMatchedContent = matchingSamples[0];
         safeArrayDelete(that.wellClasses[row][col], "well-indeterminate");
       } else if (matchingSamples.length > 1) {
         addIfNotPresent(that.wellClasses[row][col], "well-indeterminate");
       } else {
         safeArrayDelete(that.wellClasses[row][col], "well-indeterminate");
       }
-      that.patchWell(row, col, possiblyNewContent, studyID);
+      that.patchWell(row, col, possiblyMatchedContent, studyID);
     },
     function(rejectionReason) {
       bootstrapAlert(
-        "Attempting to get a list of sample IDs failed: " + rejectionReason,
+        "Attempting to get a list of sample IDs in PlateViewer.modifyWell() " +
+          "failed: " +
+          rejectionReason,
         "danger"
       );
     }
@@ -807,10 +806,10 @@ function autocomplete_search_samples(request, response) {
 
   $.when.apply($, requests).then(
     function() {
-      // The nature of arguments change based on the number of requests performed
-      // If only one request was performed, then arguments only contain the output
-      // of that request. On the other hand, if there was more than one request,
-      // then arguments is a list of results
+      // The nature of arguments change based on the number of requests
+      // performed. If only one request was performed, then arguments only
+      // contain the output of that request. On the other hand, if there was
+      // more than one request, then arguments is a list of results
       var arg = requests.length === 1 ? [arguments] : arguments;
       var samples = merge_sample_responses(arg);
       // Format the samples in the way that autocomplete needs
@@ -827,7 +826,7 @@ function autocomplete_search_samples(request, response) {
     function(jqXHR, textStatus, errorThrown) {
       bootstrapAlert(
         "Attempting to get sample IDs while filling up the autocomplete " +
-          "dropdown menu failed: " +
+          "dropdown menu in autocomplete_search_samples() failed: " +
           jqXHR.responseText,
         "danger"
       );
@@ -844,11 +843,11 @@ function autocomplete_search_samples(request, response) {
  * autocomplete_search_samples() (where this code was taken from) and
  * get_active_samples().
  *
- * NOTE that this assumes that the sample IDs in the response array are all
- * unique. If for whatever reason a sample ID is repeated in a study -- or
- * multiple studies share a sample ID -- then this function won't have a
- * problem with that, and will accordingly return an array containing duplicate
- * sample IDs. (That should never happen, though.)
+ * NOTE that this does not account for cases where there are duplicate
+ * sample IDs in the input. If for whatever reason a sample ID is repeated in a
+ * study -- or multiple studies share a sample ID -- then this function won't
+ * have a problem with that, and will accordingly return an array containing
+ * duplicate sample IDs. (That should never happen, though.)
  *
  * @param {Array} responseArray: an array of request(s') output.
  * @returns {Array} A list of the sample IDs contained within responseArray.
@@ -912,12 +911,17 @@ function get_active_samples() {
     // array of all sample IDs from all active studies.
     return $.when.apply($, requests).then(
       function() {
+        // The nature of arguments change based on the number of requests
+        // performed. If only one request was performed, then arguments only
+        // contain the output of that request. On the other hand, if there was
+        // more than one request, then arguments is a list of results
         var arg = requests.length === 1 ? [arguments] : arguments;
         return merge_sample_responses(arg);
       },
       function(jqXHR, textStatus, errorThrown) {
         bootstrapAlert(
-          "Attempting to get a list of sample IDs failed: " +
+          "Attempting to get a list of sample IDs in get_active_samples() " +
+            "failed: " +
             jqXHR.responseText,
           "danger"
         );

labcontrol/gui/templates/plate.html

@@ -213,9 +213,9 @@ <h4>Plate map</h4>
           </section>
           <!-- Hidden section added to populate a full row; otherwise, Well
               comments will not be left-justified. -->
-          <section id='plate-map-legend-indeterminate' class='legend-entry' style="visibility: hidden">
-            <div id='plate-map-legend-indeterminate-box' class='well-indeterminate legend-patch'></div>
-            <div id='plate-map-legend-indeterminate-text' class='legend-text'>
+          <section class='legend-entry' style="visibility: hidden">
+            <div class='well-indeterminate legend-patch'></div>
+            <div class='legend-text'>
              <p> Specimen ID matches more than one member of the attached
              studies; if left unresolved, will show up as "not a member of the
              attached studies" when reloading this plate</p>

labcontrol/gui/test/test_study.py

@@ -83,6 +83,8 @@ def test_get_study_samples_handler(self):
         self.assertEqual(response.code, 400)
         response = self.get('/study/1/samples?limit=-1')
         self.assertEqual(response.code, 400)
+        response = self.get('/study/1/samples?limit=-1.0')
+        self.assertEqual(response.code, 400)
         response = self.get('/study/1/samples?limit=27.0')
         self.assertEqual(response.code, 400)
         response = self.get('/study/1/samples?limit=000')