Ensure cluster_helpers.PixelSOMCluster receives the fovs argument

[alex-l-kong]

What is the purpose of this PR?

This argument was not propagated over to PixelSOMCluster, so it would always load in all the FOVs. It should only read the subset of FOVs

How did you implement your changes

Add a param fovs to PixelSOMCluster constructor to address this. When loading in the subsetted data frames, file names are verified against the fovs list provided.

[cliu72]

I think it'd be a good idea to add this functionality to CellSOMCluster as well (i.e. subset the cell table for only fovs specified in the init)

[cliu72]

This looks good to me.

Related issue - it doesn't seem like fovs is propagating to cluster_pixels either. In cluster_pixels, there is an fov argument, but the fov list is generated by looking through all fovs in the data folder.

ark-analysis/ark/phenotyping/pixel_cluster_utils.py

Line 1053 in 0b37d7b

fovs_list = find_fovs_missing_col(base_dir, data_dir, 'pixel_som_cluster')

I think it would be good to add a line after this, only keeping the fovs that are specified in the fovs argument.

Not exactly what is being addressed here - if you want, you can open a new issue for that, and I can approve this one.

[alex-l-kong]

@cliu72 if I understand correctly the list of fovs is meant to propagate across the entire pipeline. At the stage you've listed, the only FOVs that can be contained in data_dir are the ones already contained in the list of fovs, since that same list was passed to create_pixel_matrix in creating this folder. Is there a case where the user will want to specify a smaller list of fovs to assign SOM labels to? We can do that, but keep in mind we'd have to add validation checks to later steps in the pipeline to ensure, among other things, we don't attempt to consensus cluster on FOVs that don't already have SOM labels.

One improvement we can make in the meantime, however, is to avoid having to pass this list of FOVs, and instead refer to pixel_pysom.fovs instead. This list of fovs in cluster_pixels is really only meant to check if restarting functionality is needed.

[cliu72]

@cliu72 if I understand correctly the list of fovs is meant to propagate across the entire pipeline. At the stage you've listed, the only FOVs that can be contained in data_dir are the ones already contained in the list of fovs, since that same list was passed to create_pixel_matrix in creating this folder. Is there a case where the user will want to specify a smaller list of fovs to assign SOM labels to? We can do that, but keep in mind we'd have to add validation checks to later steps in the pipeline to ensure, among other things, we don't attempt to consensus cluster on FOVs that don't already have SOM labels.

One improvement we can make in the meantime, however, is to avoid having to pass this list of FOVs, and instead refer to pixel_pysom.fovs instead. This list of fovs in cluster_pixels is really only meant to check if restarting functionality is needed.

The reason that this issue even came up is because Hadeesha had pre-processed all the FOVs in his folder, then wanted to run the whole pipeline (not just training) on a subset of them. He was trying to do some time tests, but he didn't want to completely delete the pre-processed matrix folder since it took a long time to make that folder. That's why we wanted the FOVs argument to populate to the PIxelSOMcluster in the first place - before this change, it was just reading all the FOVs in the subsetted folder (which works under the paradigm that you talked about, but not if somebody wants to run the pipeline on only some of the FOVs). So to answer your question, yes, I think there is a situation in which the user will want to specify a smaller list of fovs (when testing, as Hadeesha is doing now).

I don't think it will be that hard to change this for consensus clustering either. That function also uses find_fovs_missing_col. I think it's literally a one line fix in both functions - just take the intersection of find_fovs_missing_col and the list of fovs specified by the user.

[alex-l-kong]

@cliu72 got it, just wanted to double check. I've just made the change and tested that the pipeline runs on a subset of FOVs.

[cliu72]

Looks good!