code review and suggested alterations

Author: oerc0042

isatools/create/assay_templates.py

@@ -148,7 +148,7 @@ def create_new_ontology_annotation(term_name):
     ]
 )
 
-chip_seq_dict = rna_seq_dic = OrderedDict(
+chip_seq_dict = OrderedDict(
     [
         ("measurement_type", "chromatin modification profiling"),
         ("technology_type", "nucleic acid sequencing"),

isatools/create/connectors.py

@@ -1,4 +1,5 @@
 from collections import OrderedDict
+from copy import deepcopy
 
 from isatools.create.constants import (
     BASE_FACTORS,
@@ -232,7 +233,8 @@ def generate_assay_ord_dict_from_config(datascriptor_assay_config, arm_name, epo
     )
     for name, node in datascriptor_assay_config["workflow"]:
         prepared_nodes = None
-        assert isinstance(node, dict)
+        if not isinstance(node, dict):
+            raise TypeError("Each workflow node must be a dictionary. {} was provided.".format(type(node).__name__))
         if "#replicates" in node:
             # this is a ProtocolNode
             prepared_nodes = {}
@@ -336,7 +338,7 @@ def generate_study_design(datascriptor_study_config):
             )
             arm_map[cell] = sa_plan
         source_type = Characteristic(
-            category=DEFAULT_SOURCE_TYPE.category,
+            category=deepcopy(DEFAULT_SOURCE_TYPE.category),
             value=_map_ontology_annotation(
                 arm_dict.get("subjectType", None) or study_design_config.get("subjectType", None)
             ),

isatools/create/constants.py

@@ -1,5 +1,6 @@
 # NON TREATMENT TYPES
 import os
+from functools import lru_cache
 
 import yaml
 
@@ -92,34 +93,48 @@
 LABELED_EXTRACT_PREFIX = "LBLEXTR"
 ASSAY_GRAPH_PREFIX = "AT"  # AT stands for Assay Type
 
-with open(
-    os.path.join(os.path.dirname(__file__), "..", "resources", "config", "yaml", "study-creator-config.yml")
-) as yaml_file:
-    yaml_config = yaml.load(yaml_file, Loader=yaml.FullLoader)
+_YAML_CONFIG_DIR = os.path.join(os.path.dirname(__file__), "..", "resources", "config", "yaml")
+_STUDY_CREATOR_CONFIG_PATH = os.path.join(_YAML_CONFIG_DIR, "study-creator-config.yml")
+_ASSAY_OPTIONS_CONFIG_PATH = os.path.join(_YAML_CONFIG_DIR, "assay-options.yml")
+
+
+@lru_cache(maxsize=1)
+def get_study_creator_config():
+    with open(_STUDY_CREATOR_CONFIG_PATH, encoding="utf-8") as yaml_file:
+        return yaml.load(yaml_file, Loader=yaml.FullLoader)
+
+
+@lru_cache(maxsize=1)
+def get_assay_options():
+    with open(_ASSAY_OPTIONS_CONFIG_PATH, encoding="utf-8") as yaml_file:
+        return yaml.load(yaml_file, Loader=yaml.FullLoader)
+
+
+yaml_config = get_study_creator_config()
 default_ontology_source_reference = OntologySource(**yaml_config["study"]["ontology_source_references"][1])
 
 # constants specific to the sampling plan in the study generation from the study design
 RUN_ORDER = yaml_config["study"]["protocols"][0]["parameters"][0]
 STUDY_CELL = yaml_config["study"]["protocols"][0]["parameters"][1]
 
-with open(
-    os.path.join(os.path.dirname(__file__), "..", "resources", "config", "yaml", "assay-options.yml")
-) as yaml_file:
-    assays_opts = yaml.load(yaml_file, Loader=yaml.FullLoader)
+assays_opts = get_assay_options()
 
+def get_default_source_type(
+    term="Human",
+    term_accession="http://purl.obolibrary.org/obo/NCIT_C14225",
+    term_source=default_ontology_source_reference,
+):
+    return Characteristic(
+        category=OntologyAnnotation(
+            term="Study Subject",
+            term_source=default_ontology_source_reference,
+            term_accession="http://purl.obolibrary.org/obo/NCIT_C41189",
+        ),
+        value=OntologyAnnotation(term=term, term_source=term_source, term_accession=term_accession),
+    )
 
-DEFAULT_SOURCE_TYPE = Characteristic(
-    category=OntologyAnnotation(
-        term="Study Subject",
-        term_source=default_ontology_source_reference,
-        term_accession="http://purl.obolibrary.org/obo/NCIT_C41189",
-    ),
-    value=OntologyAnnotation(
-        term="Human",
-        term_source=default_ontology_source_reference,
-        term_accession="http://purl.obolibrary.org/obo/NCIT_C14225",
-    ),
-)
+
+DEFAULT_SOURCE_TYPE = get_default_source_type()
 
 DEFAULT_LABEL = Characteristic(
     category=OntologyAnnotation(

isatools/create/model.py

@@ -9,7 +9,6 @@
 import itertools
 import json
 import logging
-import os
 import re
 import uuid
 from abc import ABC
@@ -20,7 +19,6 @@
 from numbers import Number
 
 import networkx as nx
-import yaml
 
 from isatools.create import errors
 from isatools.create.constants import (
@@ -57,6 +55,7 @@
     WASHOUT,
     ZFILL_WIDTH,
     assays_opts,
+    get_study_creator_config,
 )
 from isatools.model import (
     AcquisitionParameterDataFile,
@@ -98,6 +97,23 @@
 
 __author__ = "massi"
 
+_ALLOWED_DATA_FILE_CLASSES = {
+    RawDataFile,
+    RawSpectralDataFile,
+    ArrayDataFile,
+    FreeInductionDecayDataFile,
+    DerivedDataFile,
+    DerivedSpectralDataFile,
+    DerivedArrayDataFile,
+    ProteinAssignmentFile,
+    PeptideAssignmentFile,
+    DerivedArrayDataMatrixFile,
+    PostTranslationalModificationAssignmentFile,
+    AcquisitionParameterDataFile,
+}
+
+_DATA_FILE_CLASS_BY_NAME = {cls.__name__: cls for cls in _ALLOWED_DATA_FILE_CLASSES}
+
 
 def intersperse(lst, item):
     """
@@ -215,8 +231,9 @@ def duration(self):
     def update_duration(self, duration_value, duration_unit=None):
         if not isinstance(duration_value, Number):
             raise ValueError("duration_value must be a Number. Value provided is {0}".format(duration_value))
-        self.__duration.value = duration_value
-        self.__duration.unit = duration_unit
+        duration_factor_value = self.duration
+        duration_factor_value.value = duration_value
+        duration_factor_value.unit = duration_unit
 
 
 class Treatment(Element):
@@ -298,8 +315,9 @@ def duration(self):
     def update_duration(self, duration_value, duration_unit=None):
         if not isinstance(duration_value, Number):
             raise ValueError("duration_value must be a Number. Value provided is {0}".format(duration_value))
-        self.__duration.value = duration_value
-        self.__duration.unit = duration_unit
+        duration_factor_value = self.duration
+        duration_factor_value.value = duration_value
+        duration_factor_value.unit = duration_unit
 
 
 class StudyCell(object):
@@ -2509,8 +2527,14 @@ def _generate_isa_elements_from_node(
                                 for process in processes[::-1]
                                 if process.executes_protocol == previous_protocol_node
                             )
-                            assert isinstance(previous_process, Process)
-                            assert isinstance(item, Process)
+                            if not isinstance(previous_process, Process):
+                                raise TypeError(
+                                    "Expected Process instance for previous_process, got {}".format(
+                                        type(previous_process).__name__
+                                    )
+                                )
+                            if not isinstance(item, Process):
+                                raise TypeError("Expected Process instance for item, got {}".format(type(item).__name__))
                             log.debug("linking process {0} to process {1}".format(previous_process.name, item.name))
                             plink(previous_process, item)  # TODO check if this generates any issue
 
@@ -2530,8 +2554,14 @@ def _generate_isa_elements_from_node(
                                 for process in processes[::-1]
                                 if process.executes_protocol == previous_protocol_node
                             )
-                            assert isinstance(previous_process, Process)
-                            assert isinstance(item, Process)
+                            if not isinstance(previous_process, Process):
+                                raise TypeError(
+                                    "Expected Process instance for previous_process, got {}".format(
+                                        type(previous_process).__name__
+                                    )
+                                )
+                            if not isinstance(item, Process):
+                                raise TypeError("Expected Process instance for item, got {}".format(type(item).__name__))
                             log.debug("linking process {0} to process {1}".format(previous_process.name, item.name))
                             plink(previous_process, item)  # TODO check if this generates any issue
         return processes, other_materials, characteristic_categories, data_files, item, counter
@@ -2675,22 +2705,8 @@ def _isa_objects_factory(
                     log.debug(
                         "Assay conf. found: {}; {}; {};".format(measurement_type, technology_type, curr_assay_opt)
                     )
-                    isa_class = globals()[curr_assay_opt["raw data file"].replace(" ", "")]
-                    assert isa_class in {
-                        # expand this set if needed
-                        RawDataFile,
-                        RawSpectralDataFile,
-                        ArrayDataFile,
-                        FreeInductionDecayDataFile,
-                        DerivedDataFile,
-                        DerivedSpectralDataFile,
-                        DerivedArrayDataFile,
-                        ProteinAssignmentFile,
-                        PeptideAssignmentFile,
-                        DerivedArrayDataMatrixFile,
-                        PostTranslationalModificationAssignmentFile,
-                        AcquisitionParameterDataFile,
-                    }
+                    raw_data_file_name = curr_assay_opt["raw data file"].replace(" ", "")
+                    isa_class = _DATA_FILE_CLASS_BY_NAME.get(raw_data_file_name, RawDataFile)
                     file_extension = ".{}".format(node.extension) if node.extension else ""
                     return isa_class(
                         filename="{}_S{}_DAE_R{}_{}{}".format(
@@ -2710,12 +2726,7 @@ def generate_isa_study(self, identifier=None):
         this is the core method to return the fully populated ISA Study object from the StudyDesign
         :return: isatools.model.Study
         """
-        with open(
-            os.path.join(os.path.dirname(__file__), "..", "resources", "config", "yaml", "study-creator-config.yml")
-        ) as yaml_file:
-            config = yaml.load(yaml_file, Loader=yaml.FullLoader)
-
-        study_config = config["study"]
+        study_config = get_study_creator_config()["study"]
         study = Study(
             identifier=self.identifier or identifier or DEFAULT_STUDY_IDENTIFIER,
             title=self.name,
@@ -2808,7 +2819,8 @@ def augment_study(cls, study, study_design, in_place=False):
         :param in_place: boolean
         :return:
         """
-        assert isinstance(in_place, bool)
+        if not isinstance(in_place, bool):
+            raise TypeError("in_place must be a boolean")
         if not isinstance(study, Study):
             raise TypeError("study must be a valid Study object")
         if not isinstance(study_design, StudyDesign):
@@ -2818,7 +2830,8 @@ def augment_study(cls, study, study_design, in_place=False):
             for cell, study_assay_plan in arm.arm_map.items():
                 if study_assay_plan:
                     for assay_graph in study_assay_plan.assay_plan:
-                        assert isinstance(assay_graph, AssayGraph)
+                        if not isinstance(assay_graph, AssayGraph):
+                            raise TypeError("assay_graph must be a valid AssayGraph object")
                         if assay_graph.quality_control:
                             # CHECK the assumption here is that an assay file can unequivocally be identified
                             # by StudyCell name, corresponding AssayGraph id and measurement type
@@ -2929,7 +2942,8 @@ def _generate_quality_control_samples(
         if not isinstance(quality_control, QualityControl):
             raise TypeError()
         qc_pre = quality_control.pre_run_sample_type
-        assert isinstance(qc_pre, ProductNode)
+        if not isinstance(qc_pre, ProductNode):
+            raise TypeError("quality_control.pre_run_sample_type must be a ProductNode")
         cell_name = study_cell.name
         for i in range(qc_pre.size):
             dummy_source = QualityControlSource(
@@ -2976,7 +2990,8 @@ def _generate_quality_control_samples(
                 qc_samples_interspersed[(sample_node, interspersing_interval)].append(sample)
         log.debug("Completed interspersed samples")
         qc_post = quality_control.post_run_sample_type
-        assert isinstance(qc_post, ProductNode)
+        if not isinstance(qc_post, ProductNode):
+            raise TypeError("quality_control.post_run_sample_type must be a ProductNode")
         for i in range(qc_post.size):
             dummy_source = QualityControlSource(
                 name="SRC-QC-POST_{}_{}_{}".format(cell_name, SOURCE_QC_SOURCE_NAME, str(i).zfill(4))
@@ -2986,7 +3001,7 @@ def _generate_quality_control_samples(
                 name="SMP-QC-POST-{}_{}_{}".format(cell_name, QC_SAMPLE_NAME, str(i).zfill(4)),
                 factor_values=[],
                 characteristics=[
-                    qc_post.characteristics if i < len(qc_post.characteristics) else qc_post.characteristics[-1]
+                    qc_post.characteristics[i] if i < len(qc_post.characteristics) else qc_post.characteristics[-1]
                 ],
                 derives_from=[dummy_source],
             )

isatools/isajson/dump.py

@@ -1,10 +1,10 @@
-from json import JSONEncoder
-
-
-class ISAJSONEncoder(JSONEncoder):
-    def default(self, o):
-        if hasattr(o, "to_dict"):
-            method = getattr(o, "to_dict")
-            if callable(method):
-                return o.to_dict()
-        return JSONEncoder.default(self, o)
+from json import JSONEncoder
+
+
+class ISAJSONEncoder(JSONEncoder):
+    def default(self, o):
+        if hasattr(o, "to_dict"):
+            method = getattr(o, "to_dict")
+            if callable(method):
+                return o.to_dict()
+        return JSONEncoder.default(self, o)

isatools/isajson/load.py

@@ -1,15 +1,15 @@
-import json
-
-from isatools.model import Investigation
-
-
-def load(fp):
-    """Loads an ISA-JSON file and returns an Investigation object.
-
-    :param fp: A file-like object or a string containing the JSON data.
-    :return: An Investigation object.
-    """
-    investigation_json = json.load(fp)
-    investigation = Investigation()
-    investigation.from_dict(investigation_json)
-    return investigation
+import json
+
+from isatools.model import Investigation
+
+
+def load(fp):
+    """Loads an ISA-JSON file and returns an Investigation object.
+
+    :param fp: A file-like object or a string containing the JSON data.
+    :return: An Investigation object.
+    """
+    investigation_json = json.load(fp)
+    investigation = Investigation()
+    investigation.from_dict(investigation_json)
+    return investigation