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

Update gs_cyto_data<- to support replacement with flowSet objects. #383

Merged
merged 2 commits into from
Jun 14, 2023
Merged

Update gs_cyto_data<- to support replacement with flowSet objects. #383

merged 2 commits into from
Jun 14, 2023

Conversation

djhammill
Copy link
Contributor

This PR addresses the issue raised #372 where gs_cyto_data() replacement method no longer works for flowSet objects.

We don't want to remove support for flowSet objects just yet as many users/developers still create GatingSet objects by passing in flowSet objects - so we need to provide replacement method that supports flowSet objects too.

To fix this, we perform a class check on value and if it isn't a cytoset we convert it using flowSet_to_cytoset() before updating it in the GatingSet.

Successfully tested locally with flowSet, ncdfFlowSet and cytoset replacement values.

@@ -528,6 +528,9 @@ setReplaceMethod("flowData",signature(x="GatingSet"),function(x,value){
#' @rdname gs_cyto_data
#' @export
setReplaceMethod("gs_cyto_data",signature(x="GatingSet"),function(x,value){
if(class(value) != "cytoset") {
Copy link
Member

Choose a reason for hiding this comment

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

maybe add type checking and throw user-friendly error if it's not flowset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for the delay @mikejiang, I have added additional type checks to ensure value is a flowSet or cytoset only. I will submit another PR after this one to fix some other inconsistency issues I have encountered.

@mikejiang mikejiang merged commit 8d75837 into RGLab:master Jun 14, 2023
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