change(aliby): Consider Peter's Code Review

src/agora/io/signal.py

@@ -203,9 +203,8 @@ class Signal(BridgeH5):
                     merged = pd.DataFrame([], index=index)
         return merged
 
-    # Alan: do we need two similar properties - see below?
-    @property
-    def datasets(self):
+    @cached_property
+    def p_available(self):
         """Print data sets available in h5 file."""
         if not hasattr(self, "_available"):
             self._available = []
@@ -214,11 +213,6 @@ class Signal(BridgeH5):
         for sig in self._available:
             print(sig)
 
-    @cached_property
-    def p_available(self):
-        """Print data sets available in h5 file."""
-        self.datasets
-
     @cached_property
     def available(self):
         """Get data sets available in h5 file."""

src/agora/io/writer.py

@@ -231,7 +231,6 @@ class LinearBabyWriter(DynamicWriter):
     Assumes the edgemasks are of form ((None, tile_size, tile_size), bool).
     """
 
-    # TODO make this YAML: Alan: why?
     compression = "gzip"
     _default_tile_size = 117
     datatypes = {
@@ -320,11 +319,7 @@ class StateWriter(DynamicWriter):
     @staticmethod
     def format_values_tpback(states: list, val_name: str):
         """Unpacks a dict of state data into tp_back, trap, value."""
-        # initialise as empty lists
-        # Alan: is this initialisation necessary?
-        tp_back, trap, value = [
-            [[] for _ in states[0][val_name]] for _ in range(3)
-        ]
+
         # store results as a list of tuples
         lbl_tuples = [
             (tp_back, trap, cell_label)
@@ -335,6 +330,11 @@ class StateWriter(DynamicWriter):
         # unpack list of tuples to define variables
         if len(lbl_tuples):
             tp_back, trap, value = zip(*lbl_tuples)
+        else:
+            # set as empty lists
+            tp_back, trap, value = [
+                [[] for _ in states[0][val_name]] for _ in range(3)
+            ]
         return tp_back, trap, value
 
     @staticmethod
@@ -410,9 +410,9 @@ class StateWriter(DynamicWriter):
 
 #################### Extraction version ###############################
 class Writer(BridgeH5):
-    """Class to transform data into compatible structures."""
-
-    # Alan: when is this used?
+    """
+    Class to transform data into compatible structures.
+    Used by Extractor and Postprocessor within the pipeline."""
 
     def __init__(self, filename, flag=None, compression="gzip"):
         """
@@ -474,7 +474,7 @@ class Writer(BridgeH5):
             self.write_pd(f, path, data, compression=self.compression)
         # data is a multi-index dataframe
         elif isinstance(data, pd.MultiIndex):
-            # Alan: should we still not compress here?
+            # TODO: benchmark I/O speed when using compression
             self.write_index(f, path, data)  # , compression=self.compression)
         # data is a dictionary of dataframes
         elif isinstance(data, Dict) and np.all(

src/aliby/pipeline.py

@@ -76,14 +76,12 @@ class PipelineParameters(ParametersABC):
         postprocessing: dict (optional)
             Parameters for post-processing.
         """
-        # Alan: should 19993 be updated?
         expt_id = general.get("expt_id", 19993)
         if isinstance(expt_id, PosixPath):
             expt_id = str(expt_id)
             general["expt_id"] = expt_id
 
-        # Alan: an error message rather than a default might be better
-        directory = Path(general.get("directory", "../data"))
+        directory = Path(general["directory"])
 
         # get log files, either locally or via OMERO
         with dispatch_dataset(
@@ -174,8 +172,8 @@ class Pipeline(ProcessABC):
         "extraction",
         "postprocessing",
     ]
-    # Indicate step-writer groupings to perform special operations during step iteration
-    # Alan: replace with - specify the group in the h5 files written by each step (?)
+
+    # Specify the group in the h5 files written by each step
     writer_groups = {
         "tiler": ["trap_info"],
         "baby": ["cell_info"],
@@ -477,7 +475,7 @@ class Pipeline(ProcessABC):
                                             f"Found {steps['tiler'].n_tiles} traps in {image.name}"
                                         )
                                     elif step == "baby":
-                                        # write state and pass info to ext (Alan: what's ext?)
+                                        # write state and pass info to Extractor
                                         loaded_writers["state"].write(
                                             data=steps[
                                                 step
@@ -572,7 +570,8 @@ class Pipeline(ProcessABC):
         )
         return (traps_above_nthresh & traps_above_athresh).mean()
 
-    # Alan: can both this method and the next be deleted?
+    # FIXME: Remove this functionality. It used to be for
+    # older hdf5 file formats.
     def _load_config_from_file(
         self,
         filename: PosixPath,
@@ -587,6 +586,8 @@ class Pipeline(ProcessABC):
                     process_from[k] += 1
         return process_from, trackers_state, overwrite
 
+    # FIXME: Remove this functionality. It used to be for
+    # older hdf5 file formats.
     @staticmethod
     def legacy_get_last_tp(step: str) -> t.Callable:
         """Get last time-point in different ways depending
@@ -646,7 +647,7 @@ class Pipeline(ProcessABC):
             States of any trackers from earlier runs.
         """
         config = self.parameters.to_dict()
-        # Alan: session is never changed
+        # TODO Alan: Verify if session must be passed
         session = None
         earlystop = config["general"].get("earlystop", None)
         process_from = {k: 0 for k in self.pipeline_steps}
@@ -697,8 +698,8 @@ class Pipeline(ProcessABC):
                         )
                         config["tiler"] = steps["tiler"].parameters.to_dict()
                     except Exception:
-                        # Alan: a warning or log here?
-                        pass
+                        self._log(f"Overwriting tiling data")
+
             if config["general"]["use_explog"]:
                 meta.run()
             # add metadata not in the log file

src/aliby/tile/tiler.py

@@ -634,7 +634,7 @@ class Tiler(StepABC):
         return tile
 
 
-# Alan: do we need these as well as get_channel_index and get_channel_name?
+# FIXME: Refactor to support both channel or index
 # self._log below is not defined
 def find_channel_index(image_channels: t.List[str], channel: str):
     """

src/extraction/core/extractor.py

@@ -101,7 +101,7 @@ class Extractor(StepABC):
     Extraction follows a three-level tree structure. Channels, such as GFP, are the root level; the reduction algorithm, such as maximum projection, is the second level; the specific metric, or operation, to apply to the masks, such as mean, is the third level.
     """
 
-    # Alan: should this data be stored here or all such data in a separate file
+    # TODO Alan: Move this to a location with the SwainLab defaults
     default_meta = {
         "pixel_size": 0.236,
         "z_size": 0.6,

src/postprocessor/grouper.py

@@ -353,7 +353,6 @@ class phGrouper(NameGrouper):
         return aggregated
 
 
-# Alan: why are these separate functions?
 def concat_standard(
     path: str,
     chainer: Chainer,
@@ -475,9 +474,7 @@ class MultiGrouper:
             )
         return self._sigtable
 
-    # Alan: function seems out of place
-    # seaborn is not in pyproject.toml
-    def sigtable_plot(self) -> None:
+    def _sigtable_plot(self) -> None:
         """
         Plot number of chains for all available experiments.