Embedding step #17

JuanluOnieva-biomage · 2021-03-24T12:12:38Z

Background

Link to issue

https://biomage.atlassian.net/browse/BIOMAGE-682?atlOrigin=eyJpIjoiMTBiNGMwY2VkOTEzNDViMGFlOWMzYjAyNDA0ZWI0MzgiLCJwIjoiaiJ9

Link to staging deployment URL

Links to any Pull Requests related to this

Anything else the reviewers should know about the changes here

Before merging, changes related with Multisample support should be added (#13).

Changes

Code changes

Definition of DONE

Your changes will be ready for merging after each of the steps below have been completed:

Testing

Unit tests written
Tested locally with Inframock
Deployed to staging

To set up easy local testing with inframock, follow the instructions here: https://github.com/biomage-ltd/inframock
To deploy to the staging environment, follow the instructions here: https://github.com/biomage-ltd/biomage-utils

Documentation updates

Is all relevant documentation updated to reflect the proposed changes in this PR?

Relevant Github READMEs updated
Relevant wiki pages created/updated

Approvers

Approved by a member of the core engineering team
(UX changes) Approved by vickymorrison (this is her username, tag her if you need approval)

Just before merging:

After the PR is approved, the unstage script in here: https://github.com/biomage-ltd/biomage-utils is executed. This script cleans up your deployment to staging

Optional

Photo of a cute animal attached to this PR

alexvpickering

Looks good! Please respond to comments. Most suggestions are just coding style preferences that you are free to ignore

remoter-server/src/configureEmbedding.r

alexvpickering · 2021-03-24T19:31:38Z

remoter-server/src/configureEmbedding.r

+    # Clustering part
+    #################
+
+    message("Running clustering")
+
+    if(config$clusteringSettings$method=="louvain"){
+        clustering_method <- 1 #"Louvain"
+        clustering_resolution <- config$clusteringSettings$methodSettings[["louvain"]]["resolution"]
+
+        # HARDCODE
+        annoy.metric = "cosine"
+        scdata <- Seurat::FindNeighbors(scdata, k.param = 20, annoy.metric = annoy.metric, verbose=FALSE)
+        scdata <- Seurat::FindClusters(scdata, resolution=clustering_resolution, verbose = FALSE, algorithm = clustering_method)
+
+    }


We should probably refactor this into a seperate function to break up the function.

Let me iterate!

alexvpickering · 2021-03-24T19:37:30Z

remoter-server/src/configureEmbedding.r

+    ##########################
+    # Coloring active ident
+    ###########################
+    if ("color_pool" %in% names(scdata@misc)){
+        color_pool <- scdata@misc[['color_pool']]
+    }else{ # THIS SHOULD BE REMOVE ONCE THE EXPERIMENT HAS BEEN UPDATED WITH THE NEW VERSION OF THE DATA-INGEST 
+        color_pool <- c("#e377c2","#8c564b","#d62728","#2ca02c","#ff7f0e","#1f77b4","#f8e71c","#3957ff","#d3fe14",
+        "#c9080a","#fec7f8","#0b7b3e","#0bf0e9","#c203c8","#fd9b39","#888593","#906407","#98ba7f","#fe6794","#10b0ff",
+        "#ac7bff","#fee7c0","#964c63","#1da49c","#0ad811","#bbd9fd","#fe6cfe","#297192","#d1a09c","#78579e","#81ffad",
+        "#739400","#ca6949","#d9bf01","#646a58","#d5097e","#bb73a9","#ccf6e9","#9cb4b6","#b6a7d4","#9e8c62","#6e83c8",
+        "#01af64","#a71afd","#cfe589","#d4ccd1","#fd4109","#bf8f0e","#2f786e","#4ed1a5","#d8bb7d","#a54509","#6a9276",
+        "#a4777a","#fc12c9","#606f15","#3cc4d9","#f31c4e","#73616f","#f097c6","#fc8772","#92a6fe","#875b44","#699ab3",
+        "#94bc19","#7d5bf0","#d24dfe","#c85b74","#68ff57","#b62347","#994b91","#646b8c","#977ab4","#d694fd","#c4d5b5",
+        "#fdc4bd","#1cae05","#7bd972","#e9700a","#d08f5d","#8bb9e1","#fde945","#a29d98","#1682fb","#9ad9e0","#d6cafe",
+        "#8d8328","#b091a7","#647579","#1f8d11","#e7eafd","#b9660b","#a4a644","#fec24c","#b1168c","#188cc1","#7ab297",
+        "#4468ae","#c949a6")
+
+    }
+    scdata$color_active_ident <- color_pool[as.numeric([email protected])]
+
+    ##########################
+    # Coloring samples
+    ###########################
+    remaining.colors <- color_pool[-c(1:length(unique([email protected]$color_active_ident)))]
+    if ("type"%in%colnames([email protected])) # In that case we are in multisample experiment
+        [email protected][, "color_samples"] <- remaining.colors[as.numeric(as.factor(scdata$type))]
+    else
+        [email protected][, "color_samples"] <- remaining.colors[1]
+
+    return(scdata)
+}


Suggested change

##########################

# Coloring active ident

###########################

if ("color_pool" %in% names(scdata@misc)){

color_pool <- scdata@misc[['color_pool']]

}else{ # THIS SHOULD BE REMOVE ONCE THE EXPERIMENT HAS BEEN UPDATED WITH THE NEW VERSION OF THE DATA-INGEST

color_pool <- c("#e377c2","#8c564b","#d62728","#2ca02c","#ff7f0e","#1f77b4","#f8e71c","#3957ff","#d3fe14",

"#c9080a","#fec7f8","#0b7b3e","#0bf0e9","#c203c8","#fd9b39","#888593","#906407","#98ba7f","#fe6794","#10b0ff",

"#ac7bff","#fee7c0","#964c63","#1da49c","#0ad811","#bbd9fd","#fe6cfe","#297192","#d1a09c","#78579e","#81ffad",

"#739400","#ca6949","#d9bf01","#646a58","#d5097e","#bb73a9","#ccf6e9","#9cb4b6","#b6a7d4","#9e8c62","#6e83c8",

"#01af64","#a71afd","#cfe589","#d4ccd1","#fd4109","#bf8f0e","#2f786e","#4ed1a5","#d8bb7d","#a54509","#6a9276",

"#a4777a","#fc12c9","#606f15","#3cc4d9","#f31c4e","#73616f","#f097c6","#fc8772","#92a6fe","#875b44","#699ab3",

"#94bc19","#7d5bf0","#d24dfe","#c85b74","#68ff57","#b62347","#994b91","#646b8c","#977ab4","#d694fd","#c4d5b5",

"#fdc4bd","#1cae05","#7bd972","#e9700a","#d08f5d","#8bb9e1","#fde945","#a29d98","#1682fb","#9ad9e0","#d6cafe",

"#8d8328","#b091a7","#647579","#1f8d11","#e7eafd","#b9660b","#a4a644","#fec24c","#b1168c","#188cc1","#7ab297",

"#4468ae","#c949a6")

}

scdata$color_active_ident <- color_pool[as.numeric(scdata@active.ident)]

##########################

# Coloring samples

###########################

remaining.colors <- color_pool[-c(1:length(unique(scdata@meta.data$color_active_ident)))]

if ("type"%in%colnames(scdata@meta.data)) # In that case we are in multisample experiment

scdata@meta.data[, "color_samples"] <- remaining.colors[as.numeric(as.factor(scdata$type))]

else

scdata@meta.data[, "color_samples"] <- remaining.colors[1]

return(scdata)

}

##########################

# Coloring active ident

###########################

scdata <- color_active_ident(scdata)

return(scdata)

}

color_active_ident <- function(scdata) {

if ("color_pool" %in% names(scdata@misc)){

color_pool <- scdata@misc[['color_pool']]

} else { # THIS SHOULD BE REMOVE ONCE THE EXPERIMENT HAS BEEN UPDATED WITH THE NEW VERSION OF THE DATA-INGEST

color_pool <- c("#e377c2","#8c564b","#d62728","#2ca02c","#ff7f0e","#1f77b4","#f8e71c","#3957ff","#d3fe14",

"#c9080a","#fec7f8","#0b7b3e","#0bf0e9","#c203c8","#fd9b39","#888593","#906407","#98ba7f","#fe6794","#10b0ff",

"#ac7bff","#fee7c0","#964c63","#1da49c","#0ad811","#bbd9fd","#fe6cfe","#297192","#d1a09c","#78579e","#81ffad",

"#739400","#ca6949","#d9bf01","#646a58","#d5097e","#bb73a9","#ccf6e9","#9cb4b6","#b6a7d4","#9e8c62","#6e83c8",

"#01af64","#a71afd","#cfe589","#d4ccd1","#fd4109","#bf8f0e","#2f786e","#4ed1a5","#d8bb7d","#a54509","#6a9276",

"#a4777a","#fc12c9","#606f15","#3cc4d9","#f31c4e","#73616f","#f097c6","#fc8772","#92a6fe","#875b44","#699ab3",

"#94bc19","#7d5bf0","#d24dfe","#c85b74","#68ff57","#b62347","#994b91","#646b8c","#977ab4","#d694fd","#c4d5b5",

"#fdc4bd","#1cae05","#7bd972","#e9700a","#d08f5d","#8bb9e1","#fde945","#a29d98","#1682fb","#9ad9e0","#d6cafe",

"#8d8328","#b091a7","#647579","#1f8d11","#e7eafd","#b9660b","#a4a644","#fec24c","#b1168c","#188cc1","#7ab297",

"#4468ae","#c949a6")

}

scdata$color_active_ident <- color_pool[as.numeric(scdata@active.ident)]

##########################

# Coloring samples

###########################

remaining.colors <- color_pool[-c(1:length(unique(scdata@meta.data$color_active_ident)))]

if ("type"%in%colnames(scdata@meta.data)) # In that case we are in multisample experiment

scdata@meta.data[, "color_samples"] <- remaining.colors[as.numeric(as.factor(scdata$type))]

else

scdata@meta.data[, "color_samples"] <- remaining.colors[1]

return(scdata)

}

Possible refactor opportunity. Just a suggestion

Mm... Let me think a different refactoring since here we color two different things: sample and cluster

Finally, I decide that this is a good solution by changing the function name

Sounds good. One possibility is you could have some sort of generic color_metadata <- function(scdata, color_pool, column = c('color_samples', 'color_active_ident')) {...} that does either

Looks like Oliver created the same function here. Why don't you move it to e.g. utils.r and call it as well?

remoter-server/src/configureEmbedding.r

Co-authored-by: Alex Pickering <[email protected]>

…-runner/glob-parent-5.1.2 Bump glob-parent from 5.1.1 to 5.1.2 in /local-runner

JuanluOnieva-biomage added 4 commits March 24, 2021 10:53

First version computeEmbedding

d88ac64

Adding color feature and fix plots

63c8108

Fix computeEmbedding to configureEmbedding

d7ef253

Fix multisample support

20d85e0

alexvpickering self-requested a review March 24, 2021 17:48

alexvpickering reviewed Mar 24, 2021

View reviewed changes

JuanluOnieva-biomage and others added 2 commits March 24, 2021 22:03

Update remoter-server/src/configureEmbedding.r

2fe66b4

Co-authored-by: Alex Pickering <[email protected]>

Fix Alex comments

fef0d90

JuanluOnieva-biomage requested a review from alexvpickering March 24, 2021 22:09

alexvpickering approved these changes Mar 24, 2021

View reviewed changes

JuanluOnieva-biomage merged commit 2ae02b2 into master Mar 26, 2021

JuanluOnieva-biomage deleted the embedding-step branch March 26, 2021 08:00

alexvpickering pushed a commit that referenced this pull request Jan 4, 2023

Merge pull request #17 from biomage-org/dependabot/npm_and_yarn/local…

e907922

…-runner/glob-parent-5.1.2 Bump glob-parent from 5.1.1 to 5.1.2 in /local-runner

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Embedding step #17

Embedding step #17

JuanluOnieva-biomage commented Mar 24, 2021 •

edited

Loading

alexvpickering left a comment

alexvpickering Mar 24, 2021

JuanluOnieva-biomage Mar 24, 2021

alexvpickering Mar 24, 2021

JuanluOnieva-biomage Mar 24, 2021

JuanluOnieva-biomage Mar 24, 2021

alexvpickering Mar 24, 2021

alexvpickering Mar 25, 2021

Embedding step #17

Embedding step #17

Conversation

JuanluOnieva-biomage commented Mar 24, 2021 • edited Loading

Background

Link to issue

Link to staging deployment URL

Links to any Pull Requests related to this

Anything else the reviewers should know about the changes here

Changes

Code changes

Definition of DONE

Testing

Documentation updates

Approvers

Just before merging:

Optional

alexvpickering left a comment

Choose a reason for hiding this comment

alexvpickering Mar 24, 2021

Choose a reason for hiding this comment

JuanluOnieva-biomage Mar 24, 2021

Choose a reason for hiding this comment

alexvpickering Mar 24, 2021

Choose a reason for hiding this comment

JuanluOnieva-biomage Mar 24, 2021

Choose a reason for hiding this comment

JuanluOnieva-biomage Mar 24, 2021

Choose a reason for hiding this comment

alexvpickering Mar 24, 2021

Choose a reason for hiding this comment

alexvpickering Mar 25, 2021

Choose a reason for hiding this comment

JuanluOnieva-biomage commented Mar 24, 2021 •

edited

Loading