Skip to content

Conversation

@abombin
Copy link
Collaborator

@abombin abombin commented Jul 15, 2025

Add plot_qc_metrics function to visualize sc/spatial transcriptomics quality control metrics.
Add tests for plot_qc_metrics function.

@abombin abombin requested a review from Copilot July 15, 2025 01:47
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a new function plot_qc_metrics to generate QC metric violin plots for single-cell or spatial transcriptomics data and includes tests to validate its output.

  • Implements plot_qc_metrics in src/spac/visualization.py with optional grouping, log transformation, and customization.
  • Adds unit tests in tests/test_visualization/test_plot_qc_metrics.py to check return types and basic functionality.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tests/test_visualization/test_plot_qc_metrics.py New tests for plot_qc_metrics, verifying output types for default, log, and annotation modes
src/spac/visualization.py Introduces plot_qc_metrics, including parameter handling and Scanpy violin plotting logic
Comments suppressed due to low confidence (2)

src/spac/visualization.py:3864

  • The docstring refers to 'group_column' but the function parameter is named 'annotation'. Update the text to use 'annotation' for consistency.
        If group_column is None, returns a dictionary with keys 

tests/test_visualization/test_plot_qc_metrics.py:41

  • [nitpick] Consider adding an assertion to verify the 'axes' element in the returned dictionary for the annotation case, ensuring both 'figure' and 'axes' are returned as expected.
        self.assertIsInstance(result["A"]["figure"], Figure)

should_exist=True)

if annotation is not None:
check_annotation(adata, annotations=annotation)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @abombin , you can combine that check with the check on line 3875 (just create one list with all the annotations)

Column name in adata.obs to group the data by (default: None).
log : bool, optional
Whether to log-transform the data (default: False).
size : float, optional
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abombin , would you like to check that the size is within the correct range?

results = {}
for group in adata.obs[annotation].unique():
adata_subset = adata[adata.obs[annotation] == group]
violin_plot = sc.pl.violin(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abombin , I suggest to use "partial" to define the call once, and call it in various part of the code (e.g. here and in line 3903). This way, if you ever change it, you change it in one place.

adata = self.create_test_adata()
result = plot_qc_metrics(adata, log=True)
self.assertIsInstance(result["figure"], Figure)
self.assertTrue(isinstance(result["axes"], (np.ndarray, Axes)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abombin , would you like to check other aspects of the figure other than type of what is being returned? Something to verify that the figure actually has a valid plot in it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants