straggling changes for cosine similarity support and plotting changes to purity/ploidy plot by jrafailov · Pull Request #32 · mskilab-org/skilift

jrafailov · 2025-09-12T18:11:00Z

No description provided.

Copilot

Pull Request Overview

This PR introduces support for cosine similarity calculations in mutational signature analysis and makes improvements to purity/ploidy plot generation. The changes include adding new COSMIC signature reference files, implementing cosine similarity computation functionality, and updating plotting visualization parameters.

Adds cosine similarity computation for comparing reference and attributed signatures
Updates purity/ploidy plots with pre-computed color columns and improved visual styling
Includes new COSMIC v3.4 SBS and ID signature reference data files

Reviewed Changes

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

File	Description
inst/extdata/data/COSMIC_v3.4_SBS_GRCh37.txt	Adds COSMIC v3.4 SBS signature reference data for GRCh37
inst/extdata/data/COSMIC_v3.4_ID_GRCh37.txt	Adds COSMIC v3.4 ID signature reference data for GRCh37
R/segment-width-distribution.R	Updates plotting functions with configurable binwidth, pre-computed colors, and Cairo dependency check
R/metadata.R	Adds cosine similarity computation and integrates it into signature processing workflow

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-12T18:12:15Z

+compute_cosine_similarity <- function(
+    metadata,
+    probabilities,
+    matrix_file,
+    is_indel = FALSE,
+    is_deconstruct_sigs = FALSE,
+    reference = "hg19"
+) {


The function parameter documentation lists 'probabilities' and 'matrix_file' as paths to files, but within the function they are treated as already loaded data.tables. Either the documentation should be corrected or the function should handle file paths.

Copilot · 2025-09-12T18:12:15Z

-            geom_rect(aes(xmin = xmin, xmax = xmax, ymin = ymin, ymax = ymax, fill = 1- score)) +
-            scale_fill_scico(palette = "batlow", limits = c(0, 1), breaks = c(0, 0.25, 0.5, 0.75, 1), direction = 1, name = "Relative\nScore", guide = "none") +
+            geom_rect(aes(xmin = xmin, xmax = xmax, ymin = ymin, ymax = ymax, fill = color)) +
+            #scale_fill_scico(palette = "batlow", limits = c(0, 1), breaks = c(0, 0.25, 0.5, 0.75, 1), direction = 1, name = "Relative\nScore", guide = "none") +


The commented-out scale_fill_scico lines should be removed instead of being left as comments, as they create code clutter and confusion about the intended functionality.

Copilot · 2025-09-12T18:12:15Z

+    cosine_similarities <- numeric(ncol(weighted_prob_matrix))
+    names(cosine_similarities) <- colnames(weighted_prob_matrix)
+
+    for (i in 1:ncol(weighted_prob_matrix)) {
+        vec_a <- weighted_prob_matrix[, i]
+        vec_b <- weighted_expected[, i]
+        min_length <- min(length(vec_a), length(vec_b))
+        vec_a <- vec_a[1:min_length]
+        vec_b <- vec_b[1:min_length]
+        cosine_similarity <- sum(vec_a * vec_b) / (sqrt(sum(vec_a^2)) * sqrt(sum(vec_b^2)))
+        cosine_similarities[i] <- cosine_similarity


The cosine similarity calculation assumes matching column names between weighted_prob_matrix and weighted_expected, but uses positional indexing. This could lead to incorrect pairings if the column orders don't match. Consider using column names for safer matching.

Suggested change

cosine_similarities <- numeric(ncol(weighted_prob_matrix))

names(cosine_similarities) <- colnames(weighted_prob_matrix)

for (i in 1:ncol(weighted_prob_matrix)) {

vec_a <- weighted_prob_matrix[, i]

vec_b <- weighted_expected[, i]

min_length <- min(length(vec_a), length(vec_b))

vec_a <- vec_a[1:min_length]

vec_b <- vec_b[1:min_length]

cosine_similarity <- sum(vec_a * vec_b) / (sqrt(sum(vec_a^2)) * sqrt(sum(vec_b^2)))

cosine_similarities[i] <- cosine_similarity

cosine_similarities <- numeric(length(matching_signatures))

names(cosine_similarities) <- matching_signatures

for (sig in matching_signatures) {

if (!(sig %in% colnames(weighted_prob_matrix)) || !(sig %in% colnames(weighted_expected))) {

cosine_similarities[sig] <- NA

next

}

vec_a <- weighted_prob_matrix[, sig]

vec_b <- weighted_expected[, sig]

min_length <- min(length(vec_a), length(vec_b))

vec_a <- vec_a[1:min_length]

vec_b <- vec_b[1:min_length]

cosine_similarity <- sum(vec_a * vec_b) / (sqrt(sum(vec_a^2)) * sqrt(sum(vec_b^2)))

cosine_similarities[sig] <- cosine_similarity

Copilot · 2025-09-12T18:12:16Z

+                    if (requireNamespace("Cairo", quietly = TRUE)) {
+                        p_html <- create_purity_ploidy_plot(range, bestPloidy, bestPurity, minPurity, maxPurity, minPloidy, maxPloidy, use_geom_rect = FALSE)
+                        save_purple_sunrise_html(p_html, q, out_file_html)
+                    }


The Cairo namespace check is performed but Cairo is not actually used in the protected code block. The check should either be moved to where Cairo is actually needed or removed if not required.

Suggested change

if (requireNamespace("Cairo", quietly = TRUE)) {

p_html <- create_purity_ploidy_plot(range, bestPloidy, bestPurity, minPurity, maxPurity, minPloidy, maxPloidy, use_geom_rect = FALSE)

save_purple_sunrise_html(p_html, q, out_file_html)

}

p_html <- create_purity_ploidy_plot(range, bestPloidy, bestPurity, minPurity, maxPurity, minPloidy, maxPloidy, use_geom_rect = FALSE)

save_purple_sunrise_html(p_html, q, out_file_html)

Copilot

Pull Request Overview

Copilot reviewed 26 out of 29 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

R/lift-wrappers.R:1

Missing closing brace for the if (save_html) condition that starts around line 762. The Cairo namespace check should be nested within the save_html condition.

#' lift wrapper helper function

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-20T01:33:26Z

+shutup = function(top_level_expr, capture_output_type = "message") {
+  nr = NROW(capture_output_type)
+  do_capture_output = identical("output", capture_output_type)
+  do_capture_message =identical("message", capture_output_type)


Missing space after = operator. Should be do_capture_message = identical(...)

Suggested change

do_capture_message =identical("message", capture_output_type)

do_capture_message = identical("message", capture_output_type)

Copilot · 2025-09-20T01:33:27Z

  is_len_one = NROW(object) == 1
  is_not_valid = is_character && ! NROW(object) == 1
-  is_na = is_len_one && Skilift::is_loosely_na(object, other_nas = base::nullfile())
+  is_na = is_len_one && all(Skilift::is_loosely_na(object, other_nas = base::nullfile()))


The all() function wrapping is_loosely_na() may cause issues if is_loosely_na() returns a scalar boolean. This could break the logic for single-element character vectors.

Suggested change

is_na = is_len_one && all(Skilift::is_loosely_na(object, other_nas = base::nullfile()))

is_na = is_len_one && Skilift::is_loosely_na(object, other_nas = base::nullfile())

Copilot · 2025-09-20T01:33:27Z

+            geom_rect(aes(xmin = xmin, xmax = xmax, ymin = ymin, ymax = ymax, fill = color)) +
+            #scale_fill_scico(palette = "batlow", limits = c(0, 1), breaks = c(0, 0.25, 0.5, 0.75, 1), direction = 1, name = "Relative\nScore", guide = "none") +


Commented out code should be removed rather than left in the codebase. If this functionality needs to be preserved, consider using a conditional parameter or removing entirely.

Copilot · 2025-09-20T01:33:27Z

-            geom_raster(aes(x = ploidy, y = purity, fill = 1- score)) +
-            scale_fill_scico(palette = "batlow", limits = c(0, 1), breaks = c(0, 0.25, 0.5, 0.75, 1), direction = 1, name = "Relative\nScore", guide = "none") +
+            geom_raster(aes(x = ploidy, y = purity, fill = color)) +
+            #scale_fill_scico(palette = "batlow", limits = c(0, 1), breaks = c(0, 0.25, 0.5, 0.75, 1), direction = 1, name = "Relative\nScore", guide = "none") +


Commented out code should be removed rather than left in the codebase. If this functionality needs to be preserved, consider using a conditional parameter or removing entirely.

Suggested change

#scale_fill_scico(palette = "batlow", limits = c(0, 1), breaks = c(0, 0.25, 0.5, 0.75, 1), direction = 1, name = "Relative\nScore", guide = "none") +

…s.json automated cohort type parsing

…ubsample regardless of protein coding or not; tumor type db lookup automated..

…ootprint

* Adding var tumor_details * Specific changes for HMF dataset to use denoised_coverage_field

…dev_kh

minor cosine similarity functionality

kevinmhadi added 2 commits August 5, 2025 16:06

eln risk score, remove tmb for heme

1e23339

update

0a32130

jrafailov requested review from Copilot, kevinmhadi and shihabdider September 12, 2025 18:11

Copilot AI reviewed Sep 12, 2025

View reviewed changes

kevinmhadi added 5 commits September 16, 2025 10:51

update

065ff62

Silencing extraneous print statements during lift_all()

ca4d98b

Silencing extraneous print statements during lift_all()

f442d02

Silencing extraneous print statements during lift_all()

3df0167

collapse allelic graph before uploading.

754bb07

jrafailov requested a review from Copilot September 20, 2025 01:32

Copilot AI reviewed Sep 20, 2025

View reviewed changes

kevinmhadi and others added 17 commits September 25, 2025 15:12

tumor type wrangling through internal google sheets db and also param…

f1dc3e1

…s.json automated cohort type parsing

itdseek; keying problem with string summary; multiplicity capped to s…

f458d79

…ubsample regardless of protein coding or not; tumor type db lookup automated..

lift 2d purity ploidy plot implemented

2598348

mods to collect_complex_events for eventual dev to show in frontend.

5eef6a1

datafiles.json to arrow converter from SD

01c86a9

Under construction: complex events in filtered events table.

0dd481f

Under construction: complex events in filtered events table.

da86b28

slightly more sophisticated heuristic for determining complex event f…

9963058

…ootprint

Under construction: complex event tags

fa86cb5

bug in create_2d_purity_ploidy

8aa81f7

David's changes to dev_kh (#33)

49e1b91

* Adding var tumor_details * Specific changes for HMF dataset to use denoised_coverage_field

minor changes to silence convert_json_to_arrow output

4469bdb

Merge branch 'dev_kh' of https://github.com/mskilab-org/skilift into …

a9c3588

…dev_kh

QC flags implemented

8c6a14d

plotting changes to pp_plot

de032a4

colors supplied in data for p/p sunrise

8691713

support for cosine similarities in metadata

23f8d5e

jrafailov added 2 commits October 13, 2025 15:42

cleanup of cosine code

eca6856

guard addition to datafiles fn

fac8659

minor cosine similarity functionality

jrafailov force-pushed the jr_dev branch from 8b8c9c7 to fac8659 Compare October 13, 2025 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

straggling changes for cosine similarity support and plotting changes to purity/ploidy plot#32

straggling changes for cosine similarity support and plotting changes to purity/ploidy plot#32
jrafailov wants to merge 26 commits intomainfrom
jr_dev

jrafailov commented Sep 12, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 12, 2025

Uh oh!

Copilot AI Sep 12, 2025

Uh oh!

Copilot AI Sep 12, 2025

Uh oh!

Copilot AI Sep 12, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 20, 2025

Uh oh!

Copilot AI Sep 20, 2025

Uh oh!

Copilot AI Sep 20, 2025

Uh oh!

Copilot AI Sep 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	do_capture_message =identical("message", capture_output_type)
	do_capture_message = identical("message", capture_output_type)

	is_na = is_len_one && all(Skilift::is_loosely_na(object, other_nas = base::nullfile()))
	is_na = is_len_one && Skilift::is_loosely_na(object, other_nas = base::nullfile())

		geom_rect(aes(xmin = xmin, xmax = xmax, ymin = ymin, ymax = ymax, fill = color)) +
		#scale_fill_scico(palette = "batlow", limits = c(0, 1), breaks = c(0, 0.25, 0.5, 0.75, 1), direction = 1, name = "Relative\nScore", guide = "none") +

Conversation

jrafailov commented Sep 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 20, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants