Repository: EdinburghGenomics/clarity_scripts

Issue:
Add new rules to sample assessment in Run review
The sample assessment is using the require yield Q30 instead of the yield and %Q30 which makes it inaccurate in some cases. Change to use Yield and %Q30

- [ ] Add strategy to deal with sample that have been delivered already
- [ ] Add strategy to protect previous call when additional data is generated but not needed assuming the resulting coverage met requirement.
- [ ] Add strategy to take coverage into account.



Discussion:

Patch:
diff --git a/scripts/populate_review_step.py b/scripts/populate_review_step.py
index 3ba1948..9cccfd0 100644
--- a/scripts/populate_review_step.py
+++ b/scripts/populate_review_step.py
@@ -1,5 +1,6 @@
 #!/usr/bin/env python
 import datetime
+from egcg_core import util
 from cached_property import cached_property
 from EPPs.common import StepEPP, RestCommunicationEPP, step_argparser
 from EPPs.config import load_config
@@ -18,8 +19,8 @@ class StepPopulator(StepEPP, RestCommunicationEPP):
             if io[0]['uri'].samples[0].name == sample_name and io[1]['output-type'] == 'ResultFile'
         ]
 
-    def check_rest_data_and_artifacts(self, sample_name, selector):
-        query_args = {selector: {'sample_id': sample_name}}
+    def check_rest_data_and_artifacts(self, sample_name):
+        query_args = {'where': {'sample_id': sample_name}}
         rest_entities = self.get_documents(self.endpoint, **query_args)
         artifacts = self.output_artifacts_per_sample(sample_name=sample_name)
         if len(rest_entities) != len(artifacts):  # in sample review this will be 1, in run review this will be more
@@ -30,6 +31,18 @@ class StepPopulator(StepEPP, RestCommunicationEPP):
             )
         return rest_entities, artifacts
 
+    def delivered(self, sample_name):
+        d = {'yes': True, 'no': False}
+        query_args = {'where': {'sample_id': sample_name}}
+        sample = self.get_documents('samples', **query_args)[0]
+        return d.get(sample.get('delivered'))
+
+    def processed(self, sample_name):
+        query_args = {'where': {'sample_id': sample_name}}
+        sample = self.get_documents('samples', **query_args)[0]
+        processing_status = util.query_dict(sample, 'aggregated.most_recent_proc.status')
+        return processing_status == 'finished'
+
     def _run(self):
         raise NotImplementedError
 
@@ -51,7 +64,7 @@ class PullInfo(StepPopulator):
         self.lims.put_batch(artifacts_to_upload)
 
     def add_artifact_info(self, sample):
-        rest_entities, artifacts = self.check_rest_data_and_artifacts(sample.name, 'match')
+        rest_entities, artifacts = self.check_rest_data_and_artifacts(sample.name)
         artifacts_to_upload = set()
         for i in range(len(rest_entities)):
             for art_field, api_field in self.metrics_mapping:
@@ -83,15 +96,16 @@ class PullInfo(StepPopulator):
 
 
 class PullRunElementInfo(PullInfo):
-    endpoint = 'aggregate/run_elements'
+    endpoint = 'run_elements'
     metrics_mapping = [
         ('RE Id', 'run_element_id'),
         ('RE Nb Reads', 'passing_filter_reads'),
-        ('RE Yield', 'clean_yield_in_gb'),
-        ('RE Yield Q30', 'clean_yield_q30_in_gb'),
-        ('RE %Q30', 'clean_pc_q30'),
+        ('RE Yield', 'aggregated.clean_yield_in_gb'),
+        ('RE Yield Q30', 'aggregated.clean_yield_q30_in_gb'),
+        ('RE %Q30', 'aggregated.clean_pc_q30'),
+        ('RE Coverage', 'coverage.mean'),
         ('RE Estimated Duplicate Rate', 'lane_pc_optical_dups'),
-        ('RE %Adapter', 'pc_adapter'),
+        ('RE %Adapter', 'aggregated.pc_adaptor'),
         ('RE Review status', 'reviewed'),
         ('RE Review Comment', 'review_comments'),
         ('RE Review date', 'review_date'),
@@ -102,7 +116,6 @@ class PullRunElementInfo(PullInfo):
 
     def assess_sample(self, sample):
         artifacts_to_upload = set()
-
         artifacts = self.output_artifacts_per_sample(sample_name=sample.name)
         un_reviewed_artifacts = [a for a in artifacts if a.udf.get('RE Review status') not in ['pass', 'fail']]
         if un_reviewed_artifacts:
@@ -111,36 +124,69 @@ class PullRunElementInfo(PullInfo):
 
         # Artifacts that pass the review
         pass_artifacts = [a for a in artifacts if a.udf.get('RE Review status') == 'pass']
-
         # Artifacts that fail the review
         fail_artifacts = [a for a in artifacts if a.udf.get('RE Review status') == 'fail']
+        # Artifacts that are new
+        new_artifacts = [a for a in artifacts if a.udf.get('RE previous Useable') not in ['yes', 'no']]
 
-        target_yield = float(sample.udf.get('Yield for Quoted Coverage (Gb)'))
-        good_re_yield = sum([float(a.udf.get('RE Yield Q30')) for a in pass_artifacts])
+        # skip samples which have been delivered, mark any new REs as such, not changing older RE comments
+        if self.delivered(sample.name):
+            for a in new_artifacts:
+                a.udf['RE Useable Comment'] = 'AR: Delivered'
+                a.udf['RE Useable'] = 'no'
 
-        # Just the right amount of good yield: take it all
-        if target_yield < good_re_yield < target_yield * 2:
-            for a in pass_artifacts:
-                a.udf['RE Useable'] = 'yes'
-                a.udf['RE Useable Comment'] = 'AR: Good yield'
-            for a in fail_artifacts:
+            for a in pass_artifacts + fail_artifacts:
+                if a.udf.get('RE previous Useable Comment') and a.udf.get('RE previous Useable'):
+                    a.udf['RE Useable Comment'] = a.udf.get('RE previous Useable Comment')
+                    a.udf['RE Useable'] = a.udf.get('RE previous Useable')
+
+            artifacts_to_upload.update(artifacts)
+            return artifacts_to_upload
+
+        # skip samples which have been processed, mark any new REs as such, not changing older RE comments
+        if self.processed(sample.name):
+            for a in pass_artifacts + fail_artifacts:
+                if a.udf.get('RE previous Useable Comment') and a.udf.get('RE previous Useable'):
+                    a.udf['RE Useable Comment'] = a.udf.get('RE previous Useable Comment')
+                    a.udf['RE Useable'] = a.udf.get('RE previous Useable')
+
+            for a in new_artifacts:
+                a.udf['RE Useable Comment'] = 'AR: Sample already processed'
                 a.udf['RE Useable'] = 'no'
-                a.udf['RE Useable Comment'] = 'AR: Failed and not needed'
+
             artifacts_to_upload.update(artifacts)
+            return artifacts_to_upload
+
+        target_yield = float(sample.udf.get('Required Yield (Gb)'))
+        good_re_yield = sum([float(a.udf.get('RE Yield')) for a in pass_artifacts])
+
+        # Increase target coverage by 5% to resolve borderline cases
+        target_coverage = 1.05 * sample.udf.get('Coverage (X)')
+        obtained_coverage = float(sum([a.udf.get('RE Coverage') for a in pass_artifacts]))
 
         # Too much good yield limit to the best quality ones
-        elif good_re_yield > target_yield * 2:
+        if good_re_yield > target_yield * 2 and obtained_coverage > target_coverage:
             # Too much yield: sort the good artifact by quality
             pass_artifacts.sort(key=lambda x: x.udf.get('RE %Q30'), reverse=True)
             current_yield = 0
             for a in pass_artifacts:
-                current_yield += float(a.udf.get('RE Yield Q30'))
+                current_yield += float(a.udf.get('RE Yield'))
                 if current_yield < target_yield * 2:
                     a.udf['RE Useable'] = 'yes'
                     a.udf['RE Useable Comment'] = 'AR: Good yield'
                 else:
                     a.udf['RE Useable'] = 'no'
-                    a.udf['RE Useable Comment'] = 'AR: To much good yield'
+                    a.udf['RE Useable Comment'] = 'AR: Too much good yield'
+            for a in fail_artifacts:
+                a.udf['RE Useable'] = 'no'
+                a.udf['RE Useable Comment'] = 'AR: Failed and not needed'
+            artifacts_to_upload.update(artifacts)
+
+        # Just the right amount of good yield: take it all
+        elif target_yield < good_re_yield < target_yield * 2 or obtained_coverage > target_coverage:
+            for a in pass_artifacts:
+                a.udf['RE Useable'] = 'yes'
+                a.udf['RE Useable Comment'] = 'AR: Good yield'
             for a in fail_artifacts:
                 a.udf['RE Useable'] = 'no'
                 a.udf['RE Useable Comment'] = 'AR: Failed and not needed'
@@ -153,16 +199,16 @@ class PullRunElementInfo(PullInfo):
 
 
 class PullSampleInfo(PullInfo):
-    endpoint = 'aggregate/samples'
+    endpoint = 'samples'
     metrics_mapping = [
-        ('SR Yield (Gb)', 'clean_yield_in_gb'),
-        ('SR %Q30', 'clean_pc_q30'),
-        ('SR % Mapped', 'pc_mapped_reads'),
-        ('SR % Duplicates', 'pc_duplicate_reads'),
-        ('SR Mean Coverage', 'coverage.mean'),
-        ('SR Species Found', 'species_contamination'),
-        ('SR Sex Check Match', 'gender_match'),
-        ('SR Genotyping Match', 'genotype_match'),
+        ('SR Yield (Gb)', 'aggregated.clean_yield_in_gb'),
+        ('SR %Q30', 'aggregated.clean_pc_q30'),
+        ('SR % Mapped', 'aggregated.pc_mapped_reads'),
+        ('SR % Duplicates', 'aggregated.pc_duplicate_reads'),
+        ('SR Mean Coverage', 'aggregated.mean_coverage'),
+        ('SR Species Found', 'matching_species'),
+        ('SR Sex Check Match', 'aggregated.gender_match'),
+        ('SR Genotyping Match', 'aggregated.genotype_match'),
         ('SR Freemix', 'sample_contamination.freemix'),
         ('SR Review Status', 'reviewed'),
         ('SR Review Comments', 'review_comments'),
@@ -192,9 +238,9 @@ class PullSampleInfo(PullInfo):
 
     def field_from_entity(self, entity, api_field):
         # TODO: remove once Rest API has a sensible field for species found
-        if api_field == 'species_contamination':
-            species = entity[api_field]['contaminant_unique_mapped']
-            return ', '.join(k for k in sorted(species) if species[k] > 500)
+        if api_field == 'matching_species':
+            species = entity[api_field]
+            return ', '.join(species)
 
         return super().field_from_entity(entity, api_field)
 
@@ -214,7 +260,7 @@ class PushInfo(StepPopulator):
         _ = self.output_artifacts
         for sample in self.samples:
             self.info('Pushing data for sample %s', sample.name)
-            rest_entities, artifacts = self.check_rest_data_and_artifacts(sample.name, 'where')
+            rest_entities, artifacts = self.check_rest_data_and_artifacts(sample.name)
             rest_api_data = {}
             for e in rest_entities:
                 rest_api_data[e[self.api_id_field]] = e


Test patch:
diff --git a/tests/test_populate_review_step.py b/tests/test_populate_review_step.py
index d1b2eea..6e6c8e0 100644
--- a/tests/test_populate_review_step.py
+++ b/tests/test_populate_review_step.py
@@ -1,7 +1,7 @@
 from pyclarity_lims.entities import Artifact
 from scripts import populate_review_step as p
 from tests.test_common import TestEPP, NamedMock
-from unittest.mock import Mock, patch, PropertyMock
+from unittest.mock import Mock, patch, PropertyMock, call
 
 
 class TestPopulator(TestEPP):
@@ -13,7 +13,7 @@ class TestPopulator(TestEPP):
             self.epp_cls,
             'samples',
             new_callable=PropertyMock(
-                return_value=[NamedMock(real_name='a_sample', udf={'Yield for Quoted Coverage (Gb)': 95})]
+                return_value=[NamedMock(real_name='a_sample', udf={'Required Yield (Gb)': 95, 'Coverage (X)': 30})]
             )
         )
         self.patched_lims = patch.object(self.epp_cls, 'lims', new_callable=PropertyMock)
@@ -30,37 +30,49 @@ class TestPopulator(TestEPP):
 class TestPullRunElementInfo(TestPopulator):
     epp_cls = p.PullRunElementInfo
     fake_rest_entity = {
+        'aggregated': {'clean_yield_in_gb': 20,
+                       'clean_yield_q30_in_gb': 15,
+                       'clean_pc_q30': 75,
+                       'pc_adaptor': 1.2},
         'run_element_id': 'id',
         'passing_filter_reads': 120000000,
-        'clean_yield_in_gb': 20,
-        'clean_yield_q30_in_gb': 15,
-        'clean_pc_q30': 75,
         'lane_pc_optical_dups': 10,
-        'pc_adapter': 1.2,
         'reviewed': 'pass',
         'review_comments': 'alright',
-        'review_date': '12_02_2107_12:43:24'
+        'review_date': '12_02_2107_12:43:24',
     }
     expected_udfs = {
         'RE Id': 'id',
         'RE Nb Reads': 120000000,
         'RE Yield': 20,
         'RE Yield Q30': 15,
+        'RE Coverage': 34.2,
         'RE %Q30': 75,
         'RE Estimated Duplicate Rate': 10,
         'RE %Adapter': 1.2,
         'RE Review status': 'pass',
         'RE Review Comment': 'alright',
-        'RE Review date': '2107-02-12'
+        'RE Review date': '2107-02-12',
+        'RE Useable': 'yes',
+        'RE Useable Comment': 'AR: Good yield'
     }
 
     def test_pull(self):
+
+        patched_output_artifacts_per_sample = patch.object(
+            self.epp_cls,
+            'output_artifacts_per_sample',
+            return_value=[Mock(spec=Artifact, udf={'RE Coverage': 34.2}, samples=[NamedMock(real_name='a_sample')])]
+        )
+
         with self.patched_lims as pl, self.patched_samples, self.patched_get_docs as pg, \
-                self.patched_output_artifacts_per_sample as poa:
+                patched_output_artifacts_per_sample as poa:
             self.epp.run()
 
-            assert pg.call_count == 1
-            pg.assert_called_with(self.epp.endpoint, match={'sample_id': 'a_sample'})
+            assert pg.call_count == 3
+            assert pg.call_args_list == [call('run_elements', where={'sample_id': 'a_sample'}),
+                                         call('samples', where={'sample_id': 'a_sample'}),
+                                         call('samples', where={'sample_id': 'a_sample'})]
 
             # Check that the udfs have been added
             assert dict(poa.return_value[0].udf) == self.expected_udfs
@@ -72,16 +84,16 @@ class TestPullRunElementInfo(TestPopulator):
         def patch_output_artifact(output_artifacts):
             return patch.object(self.epp_cls, 'output_artifacts_per_sample', return_value=output_artifacts)
 
-        sample = NamedMock(real_name='a_sample', udf={'Yield for Quoted Coverage (Gb)': 95})
+        sample = NamedMock(real_name='a_sample', udf={'Required Yield (Gb)': 95, 'Coverage (X)': 30})
         patched_output_artifacts_per_sample = patch_output_artifact([
-            Mock(spec=Artifact, udf={'RE Yield Q30': 115, 'RE %Q30': 75, 'RE Review status': 'pass'}),
-            Mock(spec=Artifact, udf={'RE Yield Q30': 95, 'RE %Q30': 85, 'RE Review status': 'pass'}),
-            Mock(spec=Artifact, udf={'RE Yield Q30': 15, 'RE %Q30': 70, 'RE Review status': 'fail'}),
+            Mock(spec=Artifact, udf={'RE Yield': 115, 'RE %Q30': 75, 'RE Review status': 'pass', 'RE Coverage': 35.2}),
+            Mock(spec=Artifact, udf={'RE Yield': 95, 'RE %Q30': 85, 'RE Review status': 'pass', 'RE Coverage': 36.7}),
+            Mock(spec=Artifact, udf={'RE Yield': 15, 'RE %Q30': 70, 'RE Review status': 'fail', 'RE Coverage': 34.1}),
         ])
-        with patched_output_artifacts_per_sample as poa:
+        with patched_output_artifacts_per_sample as poa, self.patched_get_docs as pg:
             self.epp.assess_sample(sample)
             assert poa.return_value[0].udf['RE Useable'] == 'no'
-            assert poa.return_value[0].udf['RE Useable Comment'] == 'AR: To much good yield'
+            assert poa.return_value[0].udf['RE Useable Comment'] == 'AR: Too much good yield'
 
             assert poa.return_value[1].udf['RE Useable'] == 'yes'
             assert poa.return_value[1].udf['RE Useable Comment'] == 'AR: Good yield'
@@ -90,38 +102,61 @@ class TestPullRunElementInfo(TestPopulator):
             assert poa.return_value[2].udf['RE Useable Comment'] == 'AR: Failed and not needed'
 
         patched_output_artifacts_per_sample = patch_output_artifact([
-            Mock(spec=Artifact, udf={'RE Yield Q30': 115, 'RE %Q30': 85, 'RE Review status': 'pass'}),
-            Mock(spec=Artifact, udf={'RE Yield Q30': 15, 'RE %Q30': 70, 'RE Review status': 'fail'}),
+            Mock(spec=Artifact, udf={'RE Yield': 115, 'RE %Q30': 85, 'RE Review status': 'pass', 'RE Coverage': 35.2}),
+            Mock(spec=Artifact, udf={'RE Yield': 15, 'RE %Q30': 70, 'RE Review status': 'fail', 'RE Coverage': 33.6}),
         ])
-        with patched_output_artifacts_per_sample as poa:
+        with patched_output_artifacts_per_sample as poa, self.patched_get_docs as pg:
             self.epp.assess_sample(sample)
             assert poa.return_value[0].udf['RE Useable'] == 'yes'
             assert poa.return_value[0].udf['RE Useable Comment'] == 'AR: Good yield'
 
+
             assert poa.return_value[1].udf['RE Useable'] == 'no'
             assert poa.return_value[1].udf['RE Useable Comment'] == 'AR: Failed and not needed'
 
+        patched_output_artifacts_per_sample = patch_output_artifact([
+            Mock(spec=Artifact, udf={'RE Yield': 115, 'RE %Q30': 85, 'RE Review status': 'pass', 'RE Coverage': 35.2}),
+            Mock(spec=Artifact, udf={'RE Yield': 15, 'RE %Q30': 70, 'RE Review status': 'fail', 'RE Coverage': 33.6}),
+        ])
+
+        delivered = 'scripts.populate_review_step.PullRunElementInfo.delivered'
+        processed = 'scripts.populate_review_step.PullRunElementInfo.processed'
+        patched_delivered = patch(delivered, return_value=True)
+        pathed_processed = patch(processed, return_value=True)
+
+        with patched_output_artifacts_per_sample as poa, self.patched_get_docs as pg, patched_delivered:
+            self.epp.assess_sample(sample)
+            assert poa.return_value[0].udf['RE Useable'] == 'no'
+            assert poa.return_value[0].udf['RE Useable Comment'] == 'AR: Delivered'
+            assert poa.return_value[1].udf['RE Useable'] == 'no'
+            assert poa.return_value[1].udf['RE Useable Comment'] == 'AR: Delivered'
+
+        with patched_output_artifacts_per_sample as poa, self.patched_get_docs as pg, pathed_processed:
+            self.epp.assess_sample(sample)
+            assert poa.return_value[0].udf['RE Useable'] == 'no'
+            assert poa.return_value[0].udf['RE Useable Comment'] == 'AR: Sample already processed'
+            assert poa.return_value[1].udf['RE Useable'] == 'no'
+            assert poa.return_value[1].udf['RE Useable Comment'] == 'AR: Sample already processed'
+
     def test_field_from_entity(self):
         entity = {'this': {'that': 'other'}}
         assert self.epp.field_from_entity(entity, 'this.that') == 'other'
         assert entity == {'this': {'that': 'other'}}  # not changed
 
 
-class TestPullSampleInfo(TestPullRunElementInfo):
+class TestPullSampleInfo(TestPopulator):
     epp_cls = p.PullSampleInfo
     fake_rest_entity = {
         'sample_id': 'a_sample',
         'user_sample_id': 'a_user_sample_id',
         'clean_yield_in_gb': 5,
-        'clean_pc_q30': 70,
-        'pc_mapped_reads': 75,
-        'pc_duplicate_reads': 5,
-        'coverage': {'mean': 30},
-        'species_contamination': {
-            'contaminant_unique_mapped': {'Homo sapiens': 70000, 'Thingius thingy': 501, 'Sus scrofa': 499}
-        },
-        'gender_match': 'Match',
-        'genotype_match': 'Match',
+        'aggregated': {'clean_pc_q30': 70,
+                       'pc_mapped_reads': 75,
+                       'pc_duplicate_reads': 5,
+                       'mean_coverage': 30,
+                       'gender_match': 'Match',
+                       'genotype_match': 'Match'},
+        'matching_species': ['Homo sapiens', 'Thingius thingy'],
         'sample_contamination': {'freemix': 0.1},
         'reviewed': 'pass',
         'review_comments': 'alright',
@@ -162,7 +197,7 @@ class TestPullSampleInfo(TestPullRunElementInfo):
             assert poa.return_value[1].udf['SR Useable Comments'] == 'AR: Review failed'
 
     def test_field_from_entity(self):
-        obs = self.epp.field_from_entity(self.fake_rest_entity, 'species_contamination')
+        obs = self.epp.field_from_entity(self.fake_rest_entity, 'matching_species')
         assert obs == 'Homo sapiens, Thingius thingy'