after benchmarking fixes

[ned-procogia]

Happy Thanksgiving!

after benchmarking fixes - phew!

passes build tests, bioccheck, vignettes run,

[rvitancol]

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

[ned-procogia]

@@.***> – 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-procogiahttps://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 GitHubhttps://github.com/Nanostring-Biostats/GeoDiff/pull/26#issuecomment-993825578, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AV2DAO3BB6Z35EQ5XRJCEO3UQ57DHANCNFSM5IXB5P3A. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[NicoleEO]

@ned-procogia can you run the unit tests and post a snapshot of your results? Also, I noticed there are two PRs, which one should I work off of?

[ned-procogia]

@NicoleEO this is the one to use.

The results I got the other day for the testthat tests were all passed. I am re-running the tests now so that you can get a screenshot, but I don't know how informative they will be!

For the biocCheck() tests the only error was [1] "Maintainer must add package name to Watched Tags on the support site; Edit your Support Site User Profile to add Watched Tags.", which I don’t think will be an issue for you. There were some warnings here, which were stylistic choices, but I don't think any of them will pose issues. I will attach that screenshot after I have rerun it as well.

[ned-procogia]

@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

[NicoleEO]

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