Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add flowCore conversions for gates #51

Merged
merged 5 commits into from
Mar 30, 2022
Merged

feat: add flowCore conversions for gates #51

merged 5 commits into from
Mar 30, 2022

Conversation

gegnew
Copy link
Contributor

@gegnew gegnew commented May 10, 2021

Add functions to convert CE gates to flowCore gates (only applies to RectangleGate, EllipseGate, PolygonGate).

Also includes a function to apply a scale to a flowCore gate, for posting to CellEngine.

@gegnew gegnew force-pushed the ge/flowCore branch 10 times, most recently from 3e07cd1 to 3d14345 Compare May 11, 2021 16:22
@gegnew gegnew requested a review from zbjornson May 11, 2021 16:28
@gegnew
Copy link
Contributor Author

gegnew commented May 11, 2021

I got it! CircleCI, my goodness.

@gegnew gegnew requested review from zbjornson and removed request for zbjornson May 11, 2021 16:33
Copy link
Member

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, only superficial comments.

R/convertFlowCore.R Outdated Show resolved Hide resolved
R/convertFlowCore.R Outdated Show resolved Hide resolved
R/convertFlowCore.R Outdated Show resolved Hide resolved
R/convertFlowCore.R Outdated Show resolved Hide resolved
@gegnew gegnew requested a review from zbjornson May 25, 2021 16:18
R/convertFlowCore.R Outdated Show resolved Hide resolved
R/convertFlowCore.R Outdated Show resolved Hide resolved
R/fitEllipsePoints.R Outdated Show resolved Hide resolved
_pkgdown.yml Outdated Show resolved Hide resolved
R/convertFlowCore.R Outdated Show resolved Hide resolved
@gegnew gegnew force-pushed the ge/flowCore branch 3 times, most recently from 68c5f82 to a8381af Compare June 15, 2021 16:06
@gegnew gegnew requested review from zbjornson and KevinCodd June 15, 2021 16:13
Copy link
Member

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good/correct but haven't tested it out yet. Will give it a spin tomorrow.

NAMESPACE Outdated Show resolved Hide resolved
NAMESPACE Outdated Show resolved Hide resolved
NAMESPACE Outdated Show resolved Hide resolved
R/convertFlowCore.R Outdated Show resolved Hide resolved
R/convertFlowCore.R Outdated Show resolved Hide resolved
R/fitEllipsePoints.R Outdated Show resolved Hide resolved
R/fitEllipsePoints.R Outdated Show resolved Hide resolved
R/convertFlowCore.R Outdated Show resolved Hide resolved
R/convertFlowCore.R Outdated Show resolved Hide resolved
R/convertFlowCore.R Outdated Show resolved Hide resolved
#'
#' @param gate The CellEngine gate to be converted
#' @export
convertToFlowCore <- function (gate) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flowCore::transformList is their equivalent of our ScaleSet.

(Can be a later PR. Relates to #56.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this return the transformList equivalent to the ScaleSet for the experiment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Epic slow reply. Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I've implemented the scaleSetToTransformList function, I'm not so sure that convertToFlowCore should actually return the transformList, or if that should just be available from scaleSetToTransformList. I think some of these functions should be renamed, however

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice to only have to remember two function names instead of many (the three in this PR, plus transformListToScaleSet, compensationcompensation, filterListpopulation, possibly more).

Another option could be to add S3 methods for these conversions, so from.flowCore(flowCoreThing) (or as.cellengine or something) would get overloads for each of the cases that's currently in the big switch block. CellEngine objects don't have classes assigned, so currently the other way wouldn't work, but we could change that.

NAMESPACE Outdated Show resolved Hide resolved
Copy link
Member

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that took months to get through...

Reviewed closely and think I got everything this time.

}

switch(gate$type,
"RectangleGate" = flowCore::rectangleGate(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that flowCore rectangle gates are inclusive (min <= event <= max), whereas CellEngine's are exclusive (and Gating-ML's are half-open, min <= event < max), which means we should technically adjust the coordinates using nextafter. R doesn't have nextafter, but I posted one here: https://stackoverflow.com/a/69818928/1218408.

The other gate types aren't documented in this regard and I haven't looked at the flowCore source code to see.


I don't think it's worth making this change, but wanted to make a note for future reference in the off-chance it comes up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noted, thanks! I'll leave it for now

R/convertFlowCore.R Outdated Show resolved Hide resolved
}
colnames(m) <- c(gate$xChannel, gate$yChannel)
flowCore::polygonGate(filterId = gate$name, m)
},
Copy link
Member

@zbjornson zbjornson Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, these three gate types work on arcsinh and linear parameters, good.

experimentId <- "60d1685135401c03803258ad" # shared with you
rect <- cellengine::getGate(experimentId, byName("asinh rect"))
ellipse <- cellengine::getGate(experimentId, byName("asinh ellipse"))
poly <- cellengine::getGate(experimentId, byName("asinh poly"))
tl <- transformList(from=c("Er170Di", "Nd142Di"), list(arcsinhTransform(a=0,b=1/5), arcsinhTransform(a=0,b=1/5)))

file <- tempfile()
getEvents(experimentId, byName("LRS046_IL2GMCSF.fcs"), destination = file);
ff <- read.FCS(file, transformation = F)

fcrect <- toFlowCore(rect)
fcellipse <- toFlowCore(ellipse)
fcpoly <- toFlowCore(poly)

ggcyto(transform(ff, tl), aes(x=`Er170Di`, y=`Nd142Di`)) +
  ggcyto_par_set(limits = list(x=c(asinh(-5 / 5), asinh(12000 / 5)))) +
  geom_hex(bins=56) +
  geom_gate(fcrect) +
  geom_gate(fcpoly) +
  geom_gate(fcellipse)

summary(filter(transform(ff, tl), fcrect))
summary(filter(transform(ff, tl), fcellipse))
summary(filter(transform(ff, tl), fcpoly))
CellEngine FlowCore
API testing image
> summary(filter(transform(ff, tl), fcrect))
asinh rect+: 14921 of 100000 events (14.92%)
> summary(filter(transform(ff, tl), fcellipse))
asinh ellipse+: 2782 of 100000 events (2.78%)
> summary(filter(transform(ff, tl), fcpoly))
asinh poly+: 44007 of 100000 events (44.01%)

(all exactly equal)

To test lin, switch to "lin ___" for the gate names and "Time" x "Event_length".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I repeated this test for the most recent changes, and it still works. Edited names convertToFlowCore -> toFlowCore

R/convertFlowCore.R Outdated Show resolved Hide resolved
R/convertFlowCore.R Outdated Show resolved Hide resolved
tests/testthat/test-convertFlowCore.R Outdated Show resolved Hide resolved
tests/testthat/test-convertFlowCore.R Outdated Show resolved Hide resolved
tests/testthat/test-convertFlowCore.R Outdated Show resolved Hide resolved
tests/testthat/test-convertFlowCore.R Outdated Show resolved Hide resolved
tests/testthat/test-convertFlowCore.R Outdated Show resolved Hide resolved
@gegnew gegnew force-pushed the ge/flowCore branch 2 times, most recently from 7f27391 to 40980ba Compare February 10, 2022 15:51
@gegnew gegnew requested a review from zbjornson February 10, 2022 16:31
Copy link
Member

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just down to cleanup. Also note the unresolved comment above.

R/convertFromFlowCore.R Outdated Show resolved Hide resolved
R/convertFromFlowCore.R Outdated Show resolved Hide resolved
R/convertFromFlowCore.R Outdated Show resolved Hide resolved
R/convertFromFlowCore.R Outdated Show resolved Hide resolved
R/convertFromFlowCore.R Outdated Show resolved Hide resolved
R/covarToParameters.R Outdated Show resolved Hide resolved
tests/testthat/test-convertFromFlowCore.R Outdated Show resolved Hide resolved
R/convertFromFlowCore.R Outdated Show resolved Hide resolved
R/convertFromFlowCore.R Outdated Show resolved Hide resolved
tests/testthat/test-convertToFlowCore.R Show resolved Hide resolved
@gegnew
Copy link
Contributor Author

gegnew commented Mar 23, 2022

Looks good, just down to cleanup. Also note the unresolved comment above.

which one?

@zbjornson
Copy link
Member

#51 (comment)

@gegnew gegnew force-pushed the ge/flowCore branch 4 times, most recently from 2d51b79 to 319aff5 Compare March 23, 2022 19:36
@gegnew gegnew requested a review from zbjornson March 24, 2022 14:11
@zbjornson zbjornson removed the request for review from KevinCodd March 24, 2022 16:53
Copy link
Member

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more bit of cleanup and #51 (comment) is still pending.

vignettes/flowDensity.Rmd Outdated Show resolved Hide resolved
Copy link
Member

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

I'm going to squash the commits together. Commits only need to be feat if the addition is a user-visible feature (so don't need feats for the internal functions added here).

gegnew and others added 3 commits March 29, 2022 19:10
Removed plots section from _pkgdown.yml because recent versions of pkgdown fail if a section has no pages.
@zbjornson
Copy link
Member

I also fixed an R CMD CHECK warning and a failure that happens in the latest version of pkgdown while squashing the commits.

@zbjornson zbjornson merged commit 7656255 into master Mar 30, 2022
@zbjornson zbjornson deleted the ge/flowCore branch March 30, 2022 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants