Skip to content

Conversation

@khaled196
Copy link
Contributor

@khaled196 khaled196 commented Dec 2, 2025

FOR CONTRIBUTOR:

  • I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
  • License permits unrestricted use (educational + commercial)
  • This PR adds a new tool or tool collection
  • This PR updates an existing tool or tool collection
  • This PR does something else (explain below)

@khaled196 khaled196 requested a review from nsoranzo as a code owner December 2, 2025 15:58
@khaled196 khaled196 marked this pull request as draft December 2, 2025 15:58
@khaled196 khaled196 marked this pull request as ready for review December 3, 2025 13:23
khaled196 and others added 2 commits December 3, 2025 13:44
Co-authored-by: Ona <no-reply@ona.com>
<param argument="smooth" type="boolean" truevalue="TRUE" falsevalue="FALSE" checked="false" label="Smooth the graph"/>
</xml>
<xml name="plot_boundaries">
<param argument="boundaries" type="text" optional="true" value="" label="Boundaries" help="A vector of segmentation boundaries per image to plot"/>
Copy link
Member

Choose a reason for hiding this comment

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

can you please give an example of such vector in the help text?

<param argument="boundaries" type="text" optional="true" value="" label="Boundaries" help="A vector of segmentation boundaries per image to plot"/>
</xml>
<xml name="plot_molecules">
<param argument="molecules" type="text" optional="true" value="" label="Molecules" help="A vector of molecules to plot"/>
Copy link
Member

Choose a reason for hiding this comment

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

also an example vector in the help text

</xml>
<xml name="plot_molecules">
<param argument="molecules" type="text" optional="true" value="" label="Molecules" help="A vector of molecules to plot"/>
<param argument="mols_size" type="float" value="0.1" label="Point size for molecules" help="(mols.size)"/>
Copy link
Member

Choose a reason for hiding this comment

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

is there min, max?

Copy link
Contributor Author

@khaled196 khaled196 Dec 3, 2025

Choose a reason for hiding this comment

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

No min and max here. It has nmols that work as max, but this is only used for imagedimplot and not for imagefeatureplot, so I can't add it to the macros as one

<param name="mols_alpha" type="float" value="1.0" min="0.0" max="1.0" label="Alpha value for molecules" help=""/>
</xml>
<xml name="plot_nmols">
<param name="nmols" type="integer" value="1000" label="Max number of each molecule specified in `molecules` to plot" help=""/>
Copy link
Member

Choose a reason for hiding this comment

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

min, max?

Comment on lines 261 to 264
#if $method.plot.cols.cols == 'one'
cols = '$method.plot.cols.cols',
#else if $method.plot.cols.cols == 'more'
cols = c(unlist(strsplit(gsub(" ", "", '$method.plot.cols.cols_more'), ","))),
Copy link
Member

Choose a reason for hiding this comment

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

is there a select param missing in the xml with one and more values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed this option and made it a vector, no need to specify

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 pull request adds support for spatial transcriptomics visualization in Seurat by introducing two new plotting functions: ImageDimPlot for visualizing clusters or categorical groupings in a spatial context, and ImageFeaturePlot for visualizing gene expression in a spatial context. The PR also refactors some existing macro definitions to improve code reuse.

Key Changes:

  • Added ImageDimPlot and ImageFeaturePlot methods with comprehensive parameter support including FOV selection, boundary visualization, molecule overlays, and spatial-specific styling options
  • Refactored common plot parameters into reusable XML macros (e.g., plot_na_value, plot_cells, plot_boundaries)
  • Added test cases with test data files for both new visualization methods

Reviewed changes

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

File Description
tools/seurat/plot.xml Added R code templates for ImageDimPlot and ImageFeaturePlot functions (lines 249-339), added UI conditional sections for both methods (lines 797-865), added two test cases (lines 1322-1357), and wrapped FeaturePlot min/max_cutoff in conditionals (lines 405-410)
tools/seurat/macros.xml Added 14 new reusable macros for spatial plotting parameters including plot_fov, plot_boundaries, plot_molecules, plot_border, plot_dark_background, plot_crop, plot_overlap, plot_axes, plot_combine, plot_coord_fixed, plot_flip_xy, and refactored plot_na_value and plot_cells into macros
tools/seurat/test-data/imagefeatureplot.png Added test output PNG file for ImageFeaturePlot validation
tools/seurat/test-data/imagedimplot.png Added test output PNG file for ImageDimPlot validation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

<xml name="plot_molecules">
<param argument="molecules" type="text" optional="true" value="" label="Molecules" help="A vector of molecules to plot"/>
<param argument="mols_size" type="float" value="0.1" label="Point size for molecules" help="(mols.size)"/>
<param argument="mols_cols" type="text" optional="true" value="" label="Molecules Colores" help="A vector of color for molecules. The 'Set1' palette from RColorBreweris used by default (make sure they vector has the same length as the molecules)"/>
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Multiple issues in the help text: "RColorBreweris" should be "RColorBrewer is" (missing space), and "they vector" should be "the vector"

Suggested change
<param argument="mols_cols" type="text" optional="true" value="" label="Molecules Colores" help="A vector of color for molecules. The 'Set1' palette from RColorBreweris used by default (make sure they vector has the same length as the molecules)"/>
<param argument="mols_cols" type="text" optional="true" value="" label="Molecules Colores" help="A vector of color for molecules. The 'Set1' palette from RColorBrewer is used by default (make sure the vector has the same length as the molecules)"/>

Copilot uses AI. Check for mistakes.
</test>
<test expect_num_outputs="2">
<!-- test12: ImageDimPlot PNG -->
<param name="seurat_rds" ftype="rds" value="test_seurat_spatial.rds"/>
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The seurat_rds parameter is defined twice. The first definition on line 1324 with ftype="rds" value="test_seurat_spatial.rds" should be removed, as the second definition on line 1325 with the location attribute is the correct one.

Suggested change
<param name="seurat_rds" ftype="rds" value="test_seurat_spatial.rds"/>

Copilot uses AI. Check for mistakes.
khaled196 and others added 3 commits December 3, 2025 16:37
Co-authored-by: Ona <no-reply@ona.com>
Co-authored-by: Ona <no-reply@ona.com>
Co-authored-by: Ona <no-reply@ona.com>
Comment on lines 13 to 25
<requirement type="package">fit-sne</requirement>
<requirement type="package">bioconductor-limma</requirement>
<requirement type="package">bioconductor-mast</requirement>
<requirement type="package">bioconductor-deseq2</requirement>
<requirement type="package">r-svglite</requirement>
<requirement type="package">r-metap</requirement>
<requirement type="package">bioconductor-glmGamPoi</requirement>
<requirement type="package">umap-learn</requirement>
<requirement type="package">leidenalg</requirement>
<requirement type="package">r-harmony</requirement>
<requirement type="package">bioconductor-batchelor</requirement>
<requirement type="package">numpy</requirement>
<requirement type="package">pandas</requirement>
Copy link
Member

Choose a reason for hiding this comment

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

Please use fixed tool versions. Otherwise, it breaks the reproducibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am solving the conda compatibility right now. But my girpod disconnected.

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.

2 participants