RGLab / openCyto

A package that provides data analysis pipeline for flow cytometry.
GNU Affero General Public License v3.0
75 stars 29 forks source link

Minor improvements to openCyto #229

Closed DillonHammill closed 2 years ago

DillonHammill commented 2 years ago

Hi @mikejiang,

Just adding some minor changes to openCyto to make things a little easier to maintain on my end:

  1. Allowing gating process to proceed when dims are not supplied in the gatingTemplate. I have encountered this issue on numerous occasions with gating methods that don't require dims to specified. Of course, I can add dummy channel(s) in the gatingTemplate to get things to run but it makes it very unclear looking at the gatingTemplate to determine how the data has been gated. I have just bypassed subsetting the data by channels if no channels are found in the step above (i.e. empty list).

Currently there is no informative warning/error when no channels are supplied to dims, instead we get a subset out of bounds error when we subset the data below. Perhaps a warning should be added for cases where dims have been accidentally omitted. I can update this PR if you want to add a warning as below:

    if(length(channels) == 0) {
      warning(
         paste0(
           "No channels have been specified in 'dims' to gate",
           popAlias, "!"
         )
      )
    } else {
      parent_data <- parent_data[, channels] #it is more efficient to only pass the channels of interest
    }
  1. I have added a data.table method to gatingTemplate() to make it easier to convert already read in gatingTemplates into gatingTemplate objects.

Dillon

mikejiang commented 2 years ago

Allowing gating process to proceed when dims are not supplied

I am not sure about allowing empty dims,They are used to determine the population pattern and the type of gate to be generated for example 1D vs 2D gates by combining the column "pop". See https://bioconductor.org/packages/release/bioc/vignettes/openCyto/inst/doc/HowToWriteCSVTemplate.html#13_pop_=_%E2%80%9C+-+-%E2%80%9D Therefore I would rather to mandate it to avoid ramification caused by empty dims

I have added a data.table method to gatingTemplate()

Sounds good, if you can separate this into another PR

DillonHammill commented 2 years ago

Sorry for not getting back to you sooner @mikejiang. I do think that empty dims should be supported particularly since new algorithms don't adhere to the old 1D/2D gating paradigm.

If you are worried about the downstream implications of supporting this, I can take a closer look and see if there is a way around it. I can't think of implementation of an algorithm where empty dims would be used along with the pop functionality you describe above. These sorts of algorithms return cluster labels so there is no need for +/- pop functionality in these cases. Perhaps we could enforce pop=+ for entries with empty dims.

Regardless, in the meantime I have added an informative error message when dims are left empty - in part to remind me to look at this but also so users can understand what has gone wrong.