Skip to content

Conversation

@ying39purdue
Copy link
Contributor

This PR includes the combined dimensionality and spatial plots into one function which calls Visualize_2D_scatter. Additionally, it includes Liza's code for pin colors. Additionally, there is testing of the new function in test_embedded_scatter, and a single unit test added to visualize_2D_scatter for the pinned colors.

@fangliu117 fangliu117 requested a review from Copilot June 24, 2025 02:53
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 enhances the existing 2D scatter function with a user-defined color_map, and introduces a new embedded_scatter_plot to unify PCA/t-SNE/UMAP/spatial visualizations with annotation/feature coloring. It also adds a unit test verifying the custom color mapping in visualize_2D_scatter.

  • Added color_map parameter to visualize_2D_scatter and applied it in categorical plotting
  • Implemented embedded_scatter_plot for multiple embedding methods with extensive validation
  • Added test_color_map to ensure custom color mappings are correctly applied in 2D scatter

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/spac/visualization.py Added color_map, updated cluster color logic, and introduced embedded_scatter_plot
tests/test_visualization/test_visualize_2D_scatter.py Added test_color_map to verify custom color mappings in the scatter
Details src/spac/visualization.py:220

[nitpick] The color_map parameter here expects a key name, while in visualize_2D_scatter it expects a dict of label→color mappings; consider renaming one (e.g., to color_map_key) to avoid confusion.
color_map=None,

Elaboration:
The color_map parameter in function embedded_scatter_plot is documented and used as a string (a key to look up colors in adata.uns). (note: get_defined_color_map returns a dictionary mapping). However, the function it calls, visualize_2D_scatter, expects color_map to be a dictionary that maps labels directly to color values.

Why it's a Problem: This creates a confusing and inconsistent API for your library. A user will not intuitively know that the same parameter name means two different things in two different functions. The current code handles this internally by creating a new variable (color_mapping), but the public-facing function signature remains confusing.

How to Fix: Rename the parameter in embedded_scatter_plot function to be more descriptive and avoid the name collision. A better name would be color_map_key or defined_color_map.

@ying39purdue ying39purdue requested a review from fangliu117 July 1, 2025 15:36
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