Add functionality to merge altExps #243

sjspielman · 2023-12-06T16:53:24Z

Towards #150

This PR is the first PR at updating the our SCE merging to consider alternative experiments. This process has been much more of a 🎢 than we maybe had imagined, so you can expect more PRs from here. At a minimum, there will be at least 1-2 more PR here for tests which are not yet implemented.

I also want to be cautious that this PR doesn't get too long, since the code changes here are also fairly extensive. I think a good way to handle big reviews is to stack on this branch with separate PRs. Requesting a look from everybody since we may have to have some interesting discussions here! In an effort to keep this PR from getting out of hand too quickly, please focus on leaving high-level feedback about the approach for your first round of review! Thanks!

Ch-ch-ch-ch-changes

Bonus spacing updates throughout for style, as well as relevant comment updates
I added an argument include_altexp to the crux function merge_sce_list().
- This currently has a default of TRUE. Eh?
Tests are only passing right now because I set include_altexp=FALSE in all relevant spots in the unit tests. I've updated some other parts in there to lay the groundwork for circling back and adding more tests, but we'll need 1-2 more PRs to get both i) current tests working with include_altexp=TRUE, and ii) tests specifically for the new altexp functions (read on...).
The overall strategy I've taken is to handle main and alternative experiments separately - we split out the altExps at the beginning. Then, do all main exp processing and merging as before. Finally at the end, we process and merge the altExps, and then pop it into the merged object's altExp.
- Originally when working on this, I had the altExp processing happening before the SCE merge, but I kept hitting cryptic bugs, so I adopted this strategy of separate processing which seemed to be less mysteriously buggy!
Altexp processing proceeds as follows, via two new functions: prepare_altexps_for_merge() and update_altexp_assay().
- I gave up kind of quickly tbh trying manipulate sparse matrices directly, so I ended up creating new SCEs with NA values added in for rowData and counts/logcounts assays (note some TODOs in the code about handling assays). The existing altExps are therefore replaced with these new ones.
- These fresh altexps are then passed through the existing function prepare_sce_for_merge() to keep things identifiable in the final merge

…g SCEs_

…TRUE as default

…r merge, and adopt new strategy as hopefully a bug squasher - merge main and alt exps separately, and then recombine at the end

…s written yet and tests not yet passing

allyhawkins

I took a quick look, and I think I would approach this a little bit differently:

make a list of mainExp
make a list of altExp
run both lists through the existing prepare_sce_for_merge
- within this function I would add an argument for altExp = TRUE (set to FALSE by default)
- Then inside the existing prepare_sce_for_merge function you first check that all features are present in all SCE objects. If they are not shared and altExp = TRUE then you would run a separate function that prepares and modifies the assays specifically. Something like: if (missing_features & altExp){ merge_assays }.
- Before running merge_assays you could get a list of assayNames and then apply the merge_assays function to each assay. The last step would be creating the "new" altExp SCE before continuing with the rest of the prepare_sce_for_merge function (what's currently on line 372 - 385).

Let me know what you think of that

R/merge_sce_list.R

sjspielman · 2023-12-07T15:23:51Z

Thanks everyone for first round of feedback, back to it today! While reading over comments and while reviewing AlexsLemonade/scpca-nf#605, I had thoughts. Sharing them here to get my thoughts straight and get a chance for someone to tell me I'm missing something! Am I missing something or have I gotten the concept down here?

This will get the first altExp, so we are assuming that all the altExps across the merge are the same, which seems dangerous

This comment from @jashapiro brings up a good point which I had not considered. I was operating under the assumption that, because we only allow for 1 altexp at a time in a given SCE, all libraries in a project will have the "same" altexps. This is indeed dangerous! We may see some libraries within a given project (or a different integration group down the line) with adt and others with cellhash. I didn't in-depth check the library metadata to see if this is already the case, but it may indeed be in the future, so we should allow for it. Similarly, I think we already have projects where only some libraries have altexp (adt specifically).

So, I'll need to allow here for the possibility of multiple altexps in the final merged object.
In addition, we'd have to add in a fully NA altexp for any libraries that don't have an altexp that a different library in the project does have. I have no idea what kind of file size weirdness having a matrix with only NA values will cause, but we're already well down the rabbit of file size with merging in the first place, so such is life! At least adt experiments have way fewer features..!

jashapiro · 2023-12-07T15:48:16Z

because we only allow for 1 altexp at a time in a given SCE

I'll take minor issue with this statement: scpca-nf only supports 1 altExp at the moment, but scpcaTools has no such limitation! We can use merge_altExp() repeatedly to add as many altExps as we like.

Otherwise, I think your overall plan should be fine... To state it as I would see it:

Get a vector of all altexps from all libraries with altExpNames() |> union()
loop/walk through that list to make merged altExp objects and
join them them into the merged SCE.

One thing we haven't discussed, but in the merged SCEs, we probably want to update the colnames such that they are a combination of barcode and library_id if we are not doing that already, in order to keep them unique.

sjspielman · 2023-12-07T15:52:04Z

One thing we haven't discussed, but in the merged SCEs, we probably want to update the colnames such that they are a combination of barcode and library_id if we are not doing that already, in order to keep them unique.

This part is happily already there ✅

scpcaTools/R/merge_sce_list.R

Lines 237 to 238 in 4a063ff

    
           # Add `sce_name` to colnames so cell ids can be mapped to originating SCE 
        
           colnames(sce) <- glue::glue("{sce_name}-{colnames(sce)}")

Co-authored-by: Joshua Shapiro <[email protected]>

…rowData construction and new assay construction

This reverts commit bfa6846.

…nd then substitute in existing matrices. Do this for each altExp name, and for each assay within those set of altexps

…ly similar function name to the existing merge_altexp function

allyhawkins

I didn't look at the changes to the tests you made yet, but I made a few smaller comments throughout the function changes. Generally, I'm good with this approach, I just need some helpful comments so that I remember that colnames of the merged SCE object correspond to cells 🤦‍♀️

R/merge_sce_list.R

allyhawkins · 2023-12-11T19:43:08Z

R/merge_sce_list.R

+    # Ensure compatible column names
+    # (this is probably not necessary but doesn't hurt...)
+    merged_sce <- merged_sce[,merged_colnames]


Do we want to subset here... I think if we are already grabbing all columns directly from the merged SCE, we don't need this or want this.

This shouldn't be subsetting, although the syntax is the same. It's just ensuring it's all in the same order as the altexp names. This is the same all_merged_barcodes variable that was confusing.

R/merge_sce_list.R

jashapiro

A few small comments below, but I think the general plan looks good.

I see that in the meantime, Ally has put in some comments.

I too want to go through the tests, but I will do that next...

R/merge_sce_list.R

Co-authored-by: Ally Hawkins <[email protected]> Co-authored-by: Joshua Shapiro <[email protected]>

Co-authored-by: Joshua Shapiro <[email protected]>

sjspielman · 2023-12-11T21:05:21Z

I too want to go through the tests, but I will do that next...

FYI there isn't much new there. I only added some code to set up for testing altexps, and I added include_altexp=FALSE to everything else! No new actual tests yet.

I expect my next steps will be 1) PR to add rowdata+coldata, and 2) PR for testing. Sound ok?

allyhawkins

LGTM!

allyhawkins · 2023-12-12T17:26:05Z

R/merge_sce_list.R

+
+    # First we need to determine the final column names of the merged_sce (not yet made)
+    #  for use in altExp code. Later we'll apply this order to the merged_sce itself.
+    # These values are cell ids: `{sce_name}_{barcode}`


Thank you 😂

jashapiro

So this looks good, but now I see that you only updated the tests that do not include altExps.

I think it would also be useful to also include at least one test for what you expect the updated function to do. You can add edge cases in a subsequent PR, but there should be at least one test that shows that the code you included is working... You should also probably relabel the tests you updated to indicate that they are testing without altExps.

I might rearrange things to leave the tests as they were, starting with an sce_list with no altExp present (which should not require setting include_altexp), then add a separate section below to make an sce_list that does have altExps and test with both include_altexp = FALSE and include_altexp = TRUE.

tests/testthat/test-merge_sce_list.R

…nds up the same

…altexps=FALSE. Define separate sce_list_with_altexps for next steps

…ing long test suite

sjspielman · 2023-12-13T15:20:24Z

I've rearrangement the tested code as suggested in review, and I added a couple tests - first a couple tests that merge_sce_list() works as expected when altExps are present, and another test that include_altexp=FALSE indeed doesn't return an altExp. Let me know if there are more tests you'd want to see at this stage!

jashapiro

This looks good. I might add an issue for adding a few more tests for situations where there are multiple altExps, but this tests the basic functionality, which is kind of the minimal expectation. I also like that the previous tests are left mostly alone, which is a good thing for making sure you aren't introducing too many breaking changes (except directly related to the functionality you are adding).

My suggestion below are pretty minimal, with the biggest one just being that you might as well set the altExp name when you create it rather than requiring a whole separate set of code. Other than that, I think this should be good to go.

R/merge_sce_list.R

jashapiro · 2023-12-13T15:55:31Z

tests/testthat/test-merge_sce_list.R

+# vector of all expected names
+full_altexp_names <- names(altExp(sce_list_with_altexp[[2]]))


Which names are these? I think you mean feature names (colnames), and I much prefer being explicit here.

tests/testthat/test-merge_sce_list.R

jashapiro · 2023-12-13T15:57:57Z

tests/testthat/test-merge_sce_list.R

+# set altExp names themselves - all altexp are the same name here
+altexp_name <- "adt"
+sce_list_with_altexp <- sce_list_with_altexp |>
+  purrr::map(
+    \(sce) {
+      altExpNames(sce) <- altexp_name
+      sce # return the sce
+    }
+  )


See comment earlier about doing this at creation time.

jashapiro · 2023-12-13T16:00:27Z

tests/testthat/test-merge_sce_list.R

+    }
+  )
+
+test_that("merging SCEs with matching features works as expected, with altexps", {


What do you mean by matching features here? There are missing features in 1, right?

Yeah, copy/paste fail...

tests/testthat/test-metadata_to_coldata.R

tests/testthat/test-merge_sce_list.R

Co-authored-by: Joshua Shapiro <[email protected]>

tests/testthat/test-merge_sce_list.R

Co-authored-by: Joshua Shapiro <[email protected]>

sjspielman added 11 commits December 5, 2023 11:56

add comment for future us that this merge script is not about _mergin…

04bdfcd

…g SCEs_

lay the groundwork for new include_altexp argument, currently set to …

b9b7625

…TRUE as default

implement functions for updating altExp with NA features where needed

abaa73c

Update some variable names and comments, ensure altExp is prepared fo…

0e9e1c4

…r merge, and adopt new strategy as hopefully a bug squasher - merge main and alt exps separately, and then recombine at the end

fix typo

a33229d

lay some groundwork for forthcoming altexp merging tests, but no test…

76d5ec2

…s written yet and tests not yet passing

For now, turn off altexp merging in the tests to get them passing

cf0cf82

clean up a comment

382da05

batch ->sce_name argument

4a44d65

no need to re data.frame this, since we're not adding rownames here

ed2cb9a

one more test spot to turn off altexps for now

5a60129

sjspielman requested review from jashapiro and allyhawkins December 6, 2023 17:05

allyhawkins reviewed Dec 6, 2023

View reviewed changes

R/merge_sce_list.R Outdated Show resolved Hide resolved

R/merge_sce_list.R Outdated Show resolved Hide resolved

jashapiro reviewed Dec 6, 2023

View reviewed changes

R/merge_sce_list.R Outdated Show resolved Hide resolved

jashapiro reviewed Dec 6, 2023

View reviewed changes

R/merge_sce_list.R Outdated Show resolved Hide resolved

jashapiro reviewed Dec 6, 2023

View reviewed changes

R/merge_sce_list.R Outdated Show resolved Hide resolved

jashapiro reviewed Dec 6, 2023

View reviewed changes

R/merge_sce_list.R Outdated Show resolved Hide resolved

sjspielman and others added 3 commits December 7, 2023 11:27

Apply suggestions from code review

73cc8b4

Co-authored-by: Joshua Shapiro <[email protected]>

One purrr syntax bump, and changes in response to review: update new …

bd21cec

…rowData construction and new assay construction

WIP towards updated altexps strategy

bfa6846

allyhawkins mentioned this pull request Dec 7, 2023

Account for feature data when merging objects AlexsLemonade/scpca-nf#610

Merged

sjspielman added 5 commits December 11, 2023 10:12

Revert "WIP towards updated altexps strategy"

8149ed6

This reverts commit bfa6846.

Fundamental merge altExp code overhaul: Build an empty matrix first a…

3d70229

…nd then substitute in existing matrices. Do this for each altExp name, and for each assay within those set of altexps

rename function merged_altexps -> create_merged_altexps to avoid over…

c0e9da5

…ly similar function name to the existing merge_altexp function

but make it singular

f63c268

run devtools::document

5553543

sjspielman requested a review from allyhawkins December 11, 2023 19:32

allyhawkins reviewed Dec 11, 2023

View reviewed changes

jashapiro reviewed Dec 11, 2023

View reviewed changes

sjspielman and others added 6 commits December 11, 2023 15:48

Apply suggestions from code review

4689d94

Co-authored-by: Ally Hawkins <[email protected]> Co-authored-by: Joshua Shapiro <[email protected]>

Apply suggestions from code review

bdaa9cc

Co-authored-by: Joshua Shapiro <[email protected]>

merged_colnames -> all_merged_barcodes

87c7647

union and comment

5220754

respond to variable naming reviews and add missing pipe

4506626

run document

288d61b

sjspielman requested review from jashapiro and allyhawkins December 11, 2023 21:05

sjspielman mentioned this pull request Dec 11, 2023

Account for altExps when merging SCEs #150

Closed

allyhawkins approved these changes Dec 12, 2023

View reviewed changes

jashapiro reviewed Dec 12, 2023

View reviewed changes

tests/testthat/test-merge_sce_list.R Show resolved Hide resolved

sjspielman added 6 commits December 13, 2023 09:40

use regular map to avoid failures if there are no altexps; behavior e…

4eea2bf

…nds up the same

Use an actual number, no altexps in sce_list, and remove all include_…

6090faf

…altexps=FALSE. Define separate sce_list_with_altexps for next steps

Update test names, no altexps. Add some comment structure for navigat…

921b0bc

…ing long test suite

Add a test for merging with altexps

70655dc

a bit of code rearrangement

bc23a88

add test for include_altexp=FALSE when altexps are present

265f3a8

sjspielman requested a review from jashapiro December 13, 2023 15:20

jashapiro approved these changes Dec 13, 2023

View reviewed changes

sjspielman and others added 2 commits December 13, 2023 13:55

Apply suggestions from code review

c6afdeb

Co-authored-by: Joshua Shapiro <[email protected]>

Update tests in response to review

ce257ef

jashapiro reviewed Dec 13, 2023

View reviewed changes

tests/testthat/test-merge_sce_list.R Outdated Show resolved Hide resolved

Update tests/testthat/test-merge_sce_list.R

b16805d

Co-authored-by: Joshua Shapiro <[email protected]>

sjspielman merged commit 8623377 into main Dec 13, 2023
2 checks passed

sjspielman deleted the sjspielman/150-altexps-merging branch December 13, 2023 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add functionality to merge altExps #243

Add functionality to merge altExps #243

sjspielman commented Dec 6, 2023

allyhawkins left a comment

sjspielman commented Dec 7, 2023

jashapiro commented Dec 7, 2023

sjspielman commented Dec 7, 2023

allyhawkins left a comment

allyhawkins Dec 11, 2023

sjspielman Dec 11, 2023

jashapiro left a comment

sjspielman commented Dec 11, 2023

allyhawkins left a comment

allyhawkins Dec 12, 2023

sjspielman Dec 12, 2023

jashapiro left a comment

sjspielman commented Dec 13, 2023

jashapiro left a comment

jashapiro Dec 13, 2023

jashapiro Dec 13, 2023

jashapiro Dec 13, 2023

sjspielman Dec 13, 2023

		# vector of all expected names
		full_altexp_names <- names(altExp(sce_list_with_altexp[[2]]))

Add functionality to merge altExps #243

Add functionality to merge altExps #243

Conversation

sjspielman commented Dec 6, 2023

Ch-ch-ch-ch-changes

allyhawkins left a comment

Choose a reason for hiding this comment

sjspielman commented Dec 7, 2023

jashapiro commented Dec 7, 2023

sjspielman commented Dec 7, 2023

allyhawkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jashapiro left a comment

Choose a reason for hiding this comment

sjspielman commented Dec 11, 2023

allyhawkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jashapiro left a comment

Choose a reason for hiding this comment

sjspielman commented Dec 13, 2023

jashapiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment