Cheek Samples #597
Cheek Samples #597
Conversation
| ag_kit_barcode_id UUID NOT NULL PRIMARY KEY, | ||
| sample_site_last_washed_date DATE, | ||
| sample_site_last_washed_time TIME, | ||
| sample_site_last_washed_product SAMPLE_SITE_LAST_WASHED_PRODUCT_TYPE, |
There was a problem hiding this comment.
Is this allowed to be null? Under what circumstances would that be the case?
There was a problem hiding this comment.
Yes, all three of the questions are (highly) encouraged but strictly optional. As such, we allow them to be nullable in the database.
There was a problem hiding this comment.
Ok ... now I'm wondering what the meaningful difference is between (a) a completed cheek sample with no record in the ag_kit_barcodes_cheek table and (b) a completed cheek sample with a record in the ag_kit_barcodes_cheek table for which all fields except the foreign key back to the ag_kit_barcodes table are NULL. Do these two cases have two distinct meanings? What is signified by a completely empty (except for FK) record in the cheek table?
There was a problem hiding this comment.
From a metadata perspective, they do have a distinct meaning. NULL values would be reported as "not provided" while a cheek sample with no values for the questions (including NULL) would be reported as "not collected."
There was a problem hiding this comment.
Huh, that's fascinating--I don't think I really understand what these differences mean in this context, but good to know!
|
@AmandaBirmingham I think this newest commit addresses the feedback that required changes, and I've added comments to notes that either don't seem to require change or were a strictly informational question. Please let me know if I missed anything or you have any further questions or concerns. Thanks! |
| @@ -87,7 +87,7 @@ def to_api(self): | |||
| # matches that of the association between a sample and a source | |||
| class SampleInfo: | |||
| def __init__(self, sample_id, datetime_collected, site, notes, | |||
| barcode_meta=None): | |||
| barcode_meta={}): | |||
There was a problem hiding this comment.
Can this be reverted? There is an important unexpected edgecase with default objects:
>>> def foo(a, b, example={}):
... example[a] = b
... print(example)
...
>>> foo(1, 2)
{1: 2}
>>> foo(3, 4)
{1: 2, 3: 4}
>>> foo(1, 10)
{1: 10, 3: 4}
In almost all cases, the intended behavior and recommended approach is:
>>> def foo(a, b, example=None):
... if example is None:
... example = {}
... example[a] = b
... print(example)
...
>>> foo(1, 2)
{1: 2}
>>> foo(3, 4)
{3: 4}
>>> foo(1, 10)
{1: 10}
These should be caught by linters (see e.g. https://stackoverflow.com/a/73765790), which if not flagged here suggests we may need to revisit the linter configuration or linter used.
There was a problem hiding this comment.
Done - 6a31b02
This PR is intended to address the needs surrounding collecting skin samples from cheeks:
It adds Cheek as a valid sample site and adds applicable constants to the metadata pulldown process.
It adds the notion of "barcode metadata," which are fields that are strictly linked to samples, rather than implicitly linked to samples via sources (as all of the existing survey questions are).
From a UX perspective, the reason for this deviation is the temporal importance of the questions. While all metadata are temporally linked to samples, and we strive to close that gap as much as possible, these particular questions are directly related to preparation for the sample collection.
From a technical perspective, I opted to establish a new avenue of storage because neither existing method felt appropriate:
The middle ground I established is sufficient for the purpose - including data validation - and could be extended into a more robust framework if future use calls for it.
Corresponding microsetta-interface PR is biocore/microsetta-interface#334