description<- has no effect on cytoframe

[mikejiang]

  fcsfile <- system.file("extdata/CytoTrol_CytoTrol_1.fcs", package = "flowWorkspaceData")
  fr <- load_cytoframe_from_fcs(fcsfile)
> description(fr)[["newk"]] <- 1
> description(fr)[["newk"]]
NULL

Because we do not have description<- implemented/overloaded for cytoframe. So it is silently dispatching to the method of its parent class flowFrame, and attaching the keyword to description slot

> fr@description[["newk"]]
[1] 1

This is misleading. But instead of adding description<- method for cytoframe, I attempt to deprecate description methods entirely for flowFrame since it has been confusing given we already have keyword methods, which should be used as the only keywords APIs

> keyword(fr)[["newk"]] <- "d"
> keyword(fr)[["newk"]]
[1] "d"

[jacobpwagner]

I agree with the deprecation. It makes for a confusing set of APIs, particularly because of the silent failure of description<- for cytoframe.

[mikejiang]

keyword<- for cytoframe currently is not entirely compatible with flowFrame. The latter merges the new keyword list into the existing one while the former replaces the existing keyword section with the new list. This is causing the unexpected behavior in cytoframe's downstream APIs that directly inherited from flowFrame.

[jacobpwagner]

Yeah, this was a bit of a hassle for starting to adapt write.FCS in https://github.com/RGLab/flowCore/commit/eacda7f2847dc4b02495a639b4f156d60c13c489. Changing the merge behavior for flowFrame would be problematic for legacy scripts. Would it be feasible to add the merge behavior for cytoframe? Presumably if scripts were operating under the assumption of full replacement for cytoframe, that should still be fine, right?

[mikejiang]

It is actually not the canonical behavior of R's replacement method that user would expect. For example, this typical deletion operation through setting NULL won't work for the current keyword<- of flowFrame because of its partial setter

keyword(fr) <- list(kw_to_del = 1) #partial setter that only updates the existing keywords
> keyword(fr)[["kw_to_del"]]
[1] 1
> keyword(fr)[["kw_to_del"]] <- NULL
> keyword(fr)[["kw_to_del"]]
[1] 1

And actually you can't even delete any keyword without resorting to a new API.

So I am leaning toward restoring the traditional interpretation of keyword<- in flowCore to match up the current cytoframe behavior, thoughts? @gfinak @jacobpwagner

[jacobpwagner]

I agree that that makes more sense and the partial replacement/merge seemed strange to me at first based on R's typical replacement behavior, so I have no problem with full replacement being the way it works moving forward. My only concern was for backwards compatibility as what old user scripts expected to be partial replacements would now drop many keywords.

[mikejiang]

I will take your advice to protect the legacy codes and try to match cytoframe behavior to flowFrame in this case, besides we also have markernames<- that behaves similarly (it doesn't have deletion dilemma though). Perhaps it can be interpreted differently (i.e. as a update-setter) given the value is required to be a named vector or list. And we happen to have another dedicated cf_keyword_delete API (which I am also going to make some changes to be able to delete more keywords at once) already in place to take care of deletions.

We can loop back on this if decide the other way later.

[mikejiang]

BTW, the update-based setter will also cause unexpected outcome when you try to update the name of keyword, e.g.

> names(keyword(fr))[200]
[1] "k1"
> names(keyword(fr))[200] <- "k2"
> keyword(fr)[["k1"]]
[1] 1
> keyword(fr)[["k2"]]
[1] 1

In this case, instead of doing k1 --> k2, it added a new keyword k2 and preserved k1. So this calls for another dedicated API cf_keyword_rename, which is also available now.

So we are kind of ok for cytoframe in terms of the completeness of APIs for keyword modification. But still a potential issue in the long run.

[mikejiang]

keyword<- behavior for cytoframe/flowFrame is now documented as https://github.com/RGLab/flowCore/commit/0a2e2b9982b842a2a20f26a06a5e64d63c0f6712