Gate annotation in DensityScatterPlot #75

stefanppetkov · 2025-02-13T10:52:28Z

Description

This PR improves gate annotations in density scatter plots and introduces a new gate type. The changes include:

Gate Annotation Improvements

Added support for customizable annotation parameters through a list of arguments passed to geom_text
Users can now control annotation aesthetics such as:
- Text color
- Font size
- Font face
- Other text-related parameters supported by geom_text

New Feature: Quadrant Gates

Added support for quadrant gates in density scatter plots

Fixes: TECH-228

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

The changes have been tested with the following test cases:

Gate Types:
- Rectangle gates (single and multiple)
- Quadrant gates (single and faceted)
- Testing gate parameters with different data ranges
- Verifying error handling for invalid gate types
Gate Annotation Parameters:
- Testing default annotation appearance
- Customizing annotation aesthetics
- Verifying error handling for invalid annotation parameters

PR checklist:

This comment contains a description of changes (with reason)
I have performed a self-review of my own code
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
I have checked my code and documentation and corrected any misspellings
I have run R CMD check on the package and it passes without errors or warnings
I have documented any significant changes to the code in CHANGELOG.md

ludvigla

Looks great, but I will try it out on some examples to get a feel for how to use these additions

R/density_scatter_plot.R

ludvigla

I made some minor edits to the code (updated assertions and applied our styling) but everything seems to work as expected.

I know that multiple people have worked on this function now but if there's anything I'd ask for (if time allows) would be to refactor parts of the code into smaller parts and document some of the more complicated steps. Right now it's quite complicated to follow. For example, the code to create density_plot_x and density_plot_y is virtually the same except for a coord_flip(). This code could be wrapped in a small internal function.

pixelatorR.Rproj

stefanppetkov · 2025-02-20T07:41:42Z

I know that multiple people have worked on this function now but if there's anything I'd ask for (if time allows) would be to refactor parts of the code into smaller parts and document some of the more complicated steps. Right now it's quite complicated to follow. For example, the code to create density_plot_x and density_plot_y is virtually the same except for a coord_flip(). This code could be wrapped in a small internal function.

Thanks for taking the time to go through it. I completely agree that the code can be a bit hard to follow at times.

I am considering to just delay merging this a bit to try and refactor the code. This is important functionality that we will likely expand on so maintainability/readability is important.

maxkarlsson

Just one comment to add on the documentation :)
Otherwise I agree with Ludvig that a refactorization would do wonders for this function.

maxkarlsson · 2025-02-24T09:17:52Z

R/density_scatter_plot.R

+#' @param marker1 Name of first marker to plot.
+#' @param marker2 Name of second marker to plot.
+#' @param facet_vars Optional character vector of length 1 or 2 specifying variables to facet by.
+#' @param plot_gate Optional data frame containing gate parameters. If provided, `gate_type` must also be specified.


I think it would be good to specify the format of the plot_gate either here or in a details section below. This information should be provided somewhere:

A data frame with columns 'xmin', 'xmax', 'ymin', 'ymax' to plot a gate. This data frame can also contain the variables in 'facet_vars' to plot different gates in different facets.

Would also be important to specify the format of plot_gate if gate_type = "quadrant"

I expanded the documentation for plot_gate to specify the format of the dataframe.

Also split up the function into a few parts and wrapped them in internal functions, which should make the code easier to follow (I hope).

ludvigla

Fantastic work. It's much easier to follow now!

I just had one comment on the assertions that should be fixed.

R/density_scatter_plot.R

maxkarlsson

Looks great! Just add the expect_MASS utility function please.

R/density_scatter_plot.R

maxkarlsson · 2025-02-26T16:24:07Z

R/density_scatter_plot.R

+  gate_data <- gate_data %>%
+    group_by(!!!syms(facet_vars)) %>%
+    group_modify(function(.x, .y) {
+      # Get the current facet's data by filtering based on group keys
+      current_data <- if (!is.null(facet_vars)) {
+        filter_conds <- lapply(facet_vars, function(var) {
+          facet_var <- sym(var)
+          quo(!!facet_var == .y[[var]])
+        })
+        plot_data %>%
+          filter(!!!filter_conds)
+      } else {
+        plot_data
+      }


I think this might be a bit difficult to follow, but as long it works all is good!

There are a few intermediate steps which can be skipped. I refactored this part.

ludvigla

Looks great!

maxkarlsson

Looks great, well done!

stefanppetkov marked this pull request as draft February 13, 2025 11:48

stefanppetkov force-pushed the feature/tech-228 branch from b3648a3 to c3a9281 Compare February 16, 2025 16:31

stefanppetkov marked this pull request as ready for review February 16, 2025 18:42

stefanppetkov requested review from maxkarlsson and ludvigla February 16, 2025 18:43

ludvigla reviewed Feb 17, 2025

View reviewed changes

stefanppetkov requested a review from ludvigla February 19, 2025 12:58

ludvigla approved these changes Feb 19, 2025

View reviewed changes

pixelatorR.Rproj Outdated Show resolved Hide resolved

maxkarlsson reviewed Feb 24, 2025

View reviewed changes

stefanppetkov and others added 16 commits February 25, 2025 14:41

feature: add quadrant gates

1ad34e0

fix: adjust qaudrant gate annotation coorinates and add tests

f3bf4a2

chore: lint and style

116a3ca

chore: add TECH-228 changes

d6ea41a

fix: error in example

bdee1f7

fix: document ...

3dd2ad2

fix: split gate_type from plot_gate

db39476

fix: add tests, linting and styling

03dddc4

fix: styled code and updated assertions

38ee5b0

fix: refactor code

7ed976c

chore: document internal functions

7e8eae2

fix: imports and assertions

b99a770

chore: update CHANGELOG.md

1a0ad62

fix: update plot_gate documentation

04536aa

fix: lint and style

776bfb4

fix: update imports

b0815e0

stefanppetkov force-pushed the feature/tech-228 branch from 0ed6538 to b0815e0 Compare February 25, 2025 15:15

stefanppetkov requested review from ludvigla and maxkarlsson February 25, 2025 15:29

chore: CHANGELOG.md update

a22a610

ludvigla requested changes Feb 26, 2025

View reviewed changes

R/density_scatter_plot.R Outdated Show resolved Hide resolved

R/density_scatter_plot.R Outdated Show resolved Hide resolved

maxkarlsson requested changes Feb 26, 2025

View reviewed changes

stefanppetkov added 2 commits March 2, 2025 19:43

fix: set correct caller_env()

46fcc24

fix: check for MASS

2275d71

ludvigla approved these changes Mar 3, 2025

View reviewed changes

fix: refactor gate statictics

9d3daa3

stefanppetkov requested a review from maxkarlsson March 3, 2025 09:33

maxkarlsson approved these changes Mar 3, 2025

View reviewed changes

stefanppetkov merged commit 87dcfb4 into main Mar 3, 2025
7 checks passed

stefanppetkov deleted the feature/tech-228 branch March 3, 2025 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gate annotation in DensityScatterPlot #75

Gate annotation in DensityScatterPlot #75

stefanppetkov commented Feb 13, 2025 •

edited

Loading

ludvigla left a comment

ludvigla left a comment •

edited

Loading

stefanppetkov commented Feb 20, 2025

maxkarlsson left a comment

maxkarlsson Feb 24, 2025

stefanppetkov Feb 25, 2025

ludvigla left a comment

maxkarlsson left a comment

maxkarlsson Feb 26, 2025

stefanppetkov Mar 3, 2025

ludvigla left a comment

maxkarlsson left a comment

Gate annotation in DensityScatterPlot #75

Gate annotation in DensityScatterPlot #75

Conversation

stefanppetkov commented Feb 13, 2025 • edited Loading

Description

Type of change

How Has This Been Tested?

PR checklist:

ludvigla left a comment

Choose a reason for hiding this comment

ludvigla left a comment • edited Loading

Choose a reason for hiding this comment

stefanppetkov commented Feb 20, 2025

maxkarlsson left a comment

Choose a reason for hiding this comment

maxkarlsson Feb 24, 2025

Choose a reason for hiding this comment

stefanppetkov Feb 25, 2025

Choose a reason for hiding this comment

ludvigla left a comment

Choose a reason for hiding this comment

maxkarlsson left a comment

Choose a reason for hiding this comment

maxkarlsson Feb 26, 2025

Choose a reason for hiding this comment

stefanppetkov Mar 3, 2025

Choose a reason for hiding this comment

ludvigla left a comment

Choose a reason for hiding this comment

maxkarlsson left a comment

Choose a reason for hiding this comment

stefanppetkov commented Feb 13, 2025 •

edited

Loading

ludvigla left a comment •

edited

Loading