after benchmarking fixes by ned-procogia · Pull Request #26 · Nanostring-Biostats/GeoDiff

ned-procogia · 2021-11-24T21:41:57Z

Happy Thanksgiving!

after benchmarking fixes - phew!

passes build tests, bioccheck, vignettes run,

data loading and import fixes

Description update

rvitancol · 2021-12-14T17:45:12Z

@ned-procogia I realized this has not been reviewed and merged. Is this ready to be reviewed and tested?

ned-procogia · 2021-12-14T17:54:03Z

@***@***.***> – I would hold off for now. It is stable, but I think I have identified the areas responsible for the CPU maxxing out and think have some ideas for fixing that, hopefully. Thanks, Ned From: rvitancol ***@***.***> Reply to: Nanostring-Biostats/GeoDiff ***@***.***> Date: Tuesday, December 14, 2021 at 12:45 PM To: Nanostring-Biostats/GeoDiff ***@***.***> Cc: "Edward (Ned) Booker" ***@***.***>, Mention ***@***.***> Subject: External:Re: [Nanostring-Biostats/GeoDiff] after benchmarking fixes (PR #26) Warning: External. Please do not click on any links unless you trust the sender @ned-procogia<https://github.com/ned-procogia> I realized this has not been reviewed and merged. Is this ready to be reviewed and tested? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#26 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AV2DAO3BB6Z35EQ5XRJCEO3UQ57DHANCNFSM5IXB5P3A>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

made explicit what code was changed

karagorman

NBth.R - why was the parameter "split" removed (edit - nevermind, I was viewing files in a weird way)

PoisBG.R - line 143: sizefact <- apply(object, 2, mean, na.rm = TRUE) changed to sizefact <- colMeans(object). Are NAs possible? Should na.rm=TRUE be added?

^same question for lines 155, 156, 162, 244, 264, 265, 272

karagorman

ScoreTest.R - remove "#, na.rm = TRUE)" from lines 341 and 351

ned-procogia · 2022-01-19T22:56:45Z

@karagorman

"ScoreTest.R - remove "#, na.rm = TRUE)" from lines 341 and 351"

sounds good to me, I should have cleaned this up.

"NBth.R - why was the parameter "split" removed?"

I can't recall doing this, this may have been an error.

"PoisBG.R - line 143: sizefact <- apply(object, 2, mean, na.rm = TRUE) changed to sizefact <- colMeans(object). Are NAs possible? Should na.rm=TRUE be added?" - Rfast::colmeans/rowmeans/colsums/rowsums do not allow the na.rm=TRUE argument, so it should not be added here. R native row/colMeans/Sums are very slow compared to the Rfast implementations (better than apply, but not great). I think what I have works and handles nans fine

^ same question for lines 155, 156, 162, 244, 264, 265, 272

karagorman · 2022-01-20T00:05:19Z

@@ -1,4 +1,4 @@
-#' Poisson threshold model based normalization-log2 transformation for single slide or for multiple slides
+ #' Poisson threshold model based normalization-log2 transformation for single slide or for multiple slides


remove space

NicoleEO · 2022-01-20T21:03:32Z

@@ -0,0 +1,3 @@
+*.o


What's this separate .gitignore file and did you need to move these ignores to the main repo .gitignore?

This reverts commit ef30b9e.

NicoleEO

Don't forget remove the pdf
Don't worry about src-i386 folder. I double-checked and your rstudio was smart and deleted these files for us.
I am still looking at the test scripts, but have gotten through everything else

make sure NBthDE results consistent with coefNBth contrastNBth and that parallel processing works as expected

NicoleEO

@augustine-procogia

NicoleEO · 2022-01-25T23:20:57Z

@@ -322,171 +543,15 @@ setMethod(
 #'
 #' @rdname fitNBthDE-methods
 #' @aliases fitNBthDE,matrix-method


add roxygen alias for dgcmatrix

This still needs attention

@NicoleEO Added the method for dgcmatrix in my branch will be pushing it once I have addressed others

@AMAKIRI-AUGUSTINE, I was able to get the result I wanted by just using @Rdname to put in the right markdown file and @export to ensure the method gets into the markdown.

NicoleEO · 2022-01-25T23:24:57Z

+                         sizescalebythreshold=TRUE,
+                         run_parallel = TRUE,
+                         n_parallel = (parallel::detectCores()))
+ expect_error(


Add note of what you are testing for here.

NicoleEO · 2022-01-25T23:25:32Z

+
+ expect_true(all(colnames(NBthDEmod2$X) == rownames(coefmreg$estimate)))
+
+ ## 3. The user input test:statistical test, choose from c("two-sided", ">", "<")


Is this numbering correct? It goes 3., 4., then 3. again.

NicoleEO · 2022-01-25T23:26:55Z

+  expect_true(class(demoData) == "NanoStringGeoMxSet")
+  expect_true(nrow(phenoData(demoData)) == nrow(phenoData(demoData)))
+  expect_true(nrow(featureData(demoData)) == nrow(featureData(demoData)))
+  # 1 The function outputs a GeoMx S4 class with length same as length of ROIs, sizefact, in phenoData.


Update comment to match what you are testing. Technically we do not confirm length is the same here.

Noted but required to ensure the function accepts and processes the right inputs

NicoleEO · 2022-01-25T23:40:09Z

+    sizefact_mat <- matrix(rep(sizefact, n_feature), n_feature, n_sample, byrow = TRUE)
+    sizefact_mat[ind_na] <- NA
+    featfact0 <- matrix(0, n_feature, length(uniid))
+    tmp_obj = object


This still needs attention

NicoleEO · 2022-01-25T23:40:15Z

+    tmp_sizefact_mat = sizefact_mat
+    tmp_sizefact_mat[ind_na] = 0
+    for (iter in seq_len(iterations)) {
+      #featfact <- sapply(uniid, function(x) {


This still needs attention

NicoleEO · 2022-01-25T23:40:21Z

+      # apply(object[, x == id, drop = FALSE], 1, sum, na.rm = TRUE) /
+      #   apply(sizefact_mat[, x == id, drop = FALSE], 1, sum, na.rm = TRUE)
+      featfact <- sapply(uniid, function(x) {
+        rowSums(tmp_obj[, x == id, drop = FALSE], na.rm=TRUE) /


This still needs attention

NicoleEO · 2022-01-25T23:40:26Z

+
+      featfact_mat <- featfact[, id]
+      featfact_mat[ind_na] <- NA
+      tmp_featfact_mat = featfact_mat


This still needs attention

NicoleEO · 2022-01-25T23:40:32Z

+      graphics::abline(a = 0, b = 1)
    }
+
+    # tmp_mat = (countmat - countmat_expected)^2 / countmat_expected


This still needs attention

NicoleEO · 2022-01-27T22:56:03Z

Another edit to add to the list. The build system will run checks on three different OS including windows. Test chunks within unit tests using parallel package need to be set to only run on non-window OS. Here's an example of us diverting windows users to non-parallel analysis. You will want to build something of this nature in the function itself or around every unit test chunk using parallel so the unit test doesn't fail on the Bioconductor build system. https://github.com/Nanostring-Biostats/GeomxTools/blob/97959d5f38eb07b3bab380e5b82048ae368d3f72/R/NanoStringGeoMxSet-de.R#L80

ned-procogia and others added 25 commits October 26, 2021 09:27

Create R-CI.yml

ed58d85

Update R-CI.yml

8ec05bd

Update R-CI.yml

9dd2608

ned edits

1ef1c64

Delete .github/workflows directory

ee68e67

Merge branch 'dev' into main

a08aea6

Update NEWS.md

fca869e

Update DESCRIPTION

08f8f66

align with BiocCheck()

0729159

i wonder what this does

97bc524

update tests to check for sparse matrix

dfc9fbd

added unit tests for ned changes

b9db053

Update DESCRIPTION

c7fa488

handle large matrices

2233d55

data loading and import fixes

d6a4104

Merge branch 'main' into description-update

bc60223

Merge pull request #8 from ned-procogia/description-update

f55c6ad

data loading and import fixes

something something

914f7ed

cleanup

dec8063

Merge pull request #9 from ned-procogia/description-update

9ca7003

Description update

Add files via upload

d237b94

more little fixes

8f45dfb

bugfixes to pass tests

d36ba9b

that is a whoopsie

4a5ec05

Update NBthDEOptPara.cpp

4be42b7

ned-procogia added 3 commits December 21, 2021 14:02

rcpp ec2 optimizations

446d7d9

Update NBthDEOptPara.cpp

e7bd090

Update NBthDEOptPara.cpp

c2e5b0a

ned-procogia added 2 commits January 19, 2022 09:40

clarified news

1262168

made explicit what code was changed

add new line at bottom

4c28266

karagorman reviewed Jan 19, 2022

View reviewed changes

karagorman suggested changes Jan 19, 2022

View reviewed changes

karagorman reviewed Jan 20, 2022

View reviewed changes

remove space

00d7854

NicoleEO reviewed Jan 20, 2022

View reviewed changes

ned-procogia added 4 commits January 20, 2022 14:01

reformat code

ef30b9e

Revert "reformat code"

8e85d20

This reverts commit ef30b9e.

one space tab

faecfa4

re-indent tests

f2029dc

NicoleEO suggested changes Jan 21, 2022

View reviewed changes

ned-procogia added 15 commits January 20, 2022 22:00

Delete NBthDE_model_description.pdf

57f63ff

change = to <-

deda92a

Update NBthDE.R

ac0141f

Update ScoreTest.R

385f996

Update test-fitNBthDE.R

7603be1

Update NBthDE.R

710f794

Update NBthDEOptPara.cpp

bb242fb

added tests

2961e5f

make sure NBthDE results consistent with coefNBth contrastNBth and that parallel processing works as expected

Update test-fitNBthDE.R

3756cf1

Update NBthDEOptPara.cpp

390bf0e

Update test-fitNBthDE.R

4716041

Update DESCRIPTION

3f840b6

Add files via upload

c23d738

Update test-fitNBthDE.R

8fd3b52

Update data.R

de8cc52

NicoleEO suggested changes Jan 25, 2022

View reviewed changes

		@@ -1,4 +1,4 @@
		#' Poisson threshold model based normalization-log2 transformation for single slide or for multiple slides
		#' Poisson threshold model based normalization-log2 transformation for single slide or for multiple slides


		expect_true(all(colnames(NBthDEmod2$X) == rownames(coefmreg$estimate)))

		## 3. The user input test:statistical test, choose from c("two-sided", ">", "<")

Uh oh!

Conversation

ned-procogia commented Nov 24, 2021

Uh oh!

rvitancol commented Dec 14, 2021

Uh oh!

ned-procogia commented Dec 14, 2021 via email

Uh oh!

karagorman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karagorman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ned-procogia commented Jan 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicoleEO left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NicoleEO left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicoleEO commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

karagorman left a comment •

edited

Loading

karagorman left a comment •

edited

Loading

NicoleEO commented Jan 27, 2022 •

edited

Loading