search

BIMSBbioinfo / scregseg

Single-cell regulatory landscape segmentation
GNU General Public License v3.0
5 stars 2 forks source link

Facilitate passing on of keword argument col_colors to sns.heatmap an… #3

Closed prauten closed 2 years ago

prauten commented 2 years ago

…d enable matching via cell id/sample name of function plot_cell_state_association.

For me, it is of interest to see how the cell states associate with meta information of my samples (e.g., stimulation/control, different batches, etc.). Hence, I tried to pass a keyword argument to sns.heatmap in the plot_cell_state_association function call.

I realized that passing keyword arguments to sns.heatmap was not enabled. Only positional arguments could be passed.

To change this, I changed *kwargs to **kwargs in the function call.

Also, in the resulting plot, currently, cells are given sequential numbers as sample names instead of their cellular barcodes (e.g., in the emission plot). As I consider matching the colors with the cells less error-prone using the sample names, I made it an optional parameter of the function (use_cell_ids).

This is what my changes look like in an example where use_cell_ids is set to True and colors matching the cell ids are passed on as a keyword argument to plot_cell_state_association (col_colors): Passing_on_col_colors_to_heatmap_matched_on_sample_names

If you don't pass on keyword arguments and don't set the optional novel keyword argument use_cell_ids to True the plots look exactly as before:

Original_implementation

wkopp commented 2 years ago

Thank you, Pia,

yes, kwargs should have been used with * instead of in the function call. That seems to be a bug. And yes, it makes sense to plot the cell labels. I think it would even be better to always just plot the cell labels and just fall back on numbers only if the labels are not available.

Since these are separate issues, may I ask you to create a separate pull request for each issue/bug/feature. So I'd suggest to revert the use_cell_ids changes from this pull request so that it only concerns with the **kwargs issue. Then make a separate pull request for plotting the cell labels. For that I would suggest to just plot the cell labels without introducing the use_cell_ids argument.

Thank you very much for you contributions!

prauten commented 2 years ago

Hi Wolfgang,

No worries. I am glad I can contribute something here!

I reverted the changes related to the cell labels in the plot and only retained the bug fix for the kwargs argument. I will open a separate pull request for the cell labels.

Best, Pia