search

stan-dev / posterior

The posterior R package
https://mc-stan.org/posterior/
Other
167 stars 24 forks source link

Adjust rvar S4 class definition #267

Closed mjskay closed 1 year ago

mjskay commented 1 year ago

From a discussion on {jsonlite} (below), we may need to change the S4 class def of rvar to avoid redefining the S4 class definition of vctrs_vctr. See comment:

          @mjskay I'm not sure this is better because then `getClassDef("vctrs_vctr")` reports a character class:
Virtual Class "vctrs_vctr" [in ".GlobalEnv"]

Slots:

Name:   .S3Class
Class: character

Extends: "oldClass"

Known Subclasses: "rvar"

Would the following work for you? This way you're not declaring anything about our vctrs classes:

setOldClass("rvar")

Just guessing as I don't know much about S4 either.

Originally posted by @lionel- in https://github.com/jeroen/jsonlite/issues/408#issuecomment-1384450379

mjskay commented 1 year ago

@lionel- With a quick test, setOldClass("rvar") doesn't break any of our existing tests, so that could work AFAICT.

I do wonder how future-proof that is though. If some future extension writes S4 code that supports vctrs, rvar would not be able to take advantage. With some quick testing, I am pretty sure setOldClass(c("rvar", "vctrs_vctr")) won't redefine "vctrs_vctr" if "vctrs_vctr" is already defined --- the character superclass is just the default superclass assigned when none is specified. So, another solution might be for {vctrs} to make its own call to setOldClass() for "vctrs_vctr". Then you could define a sensible superclass for it instead of the default "character", and a call to setOldClass(c("rvar", "vctrs_vctr")) in {posterior} should not redefine it.

One downside to that setup is probably that the S4 superclass of "vctrs_vctr" would have to be fixed (I think?), which is obviously contrary to how "vctrs_vctr" is used in S3, since different vctrs_vctr subtypes have different S3 superclasses. However, I think that is unavoidable if "vctrs_vctr" is ever given an S4 class definition. The other alternative might be to have no one (inside or outside {vctrs}) ever call setOldClass() on "vctrs_vctr" or a subclass of it. In which case, the advice on setOldClass() in the vctrs s3-vector vignette would need to be updated.

paul-buerkner commented 1 year ago

I have not enough experience with S4 classes so I just leave the decision of what to do up to you @mjskay

mjskay commented 1 year ago

The solution to #269 is going to be to drop "list" from the class definition of rvar anyway, which incidentally will solve this issue as well by making the incantation setOldClass(c("rvar","vctrs_vctr")). That is also consistent with the {vctrs} documentation. So, I am closing this for and will revisit later if the vctrs folks change their recommendations for use of setOldClass() in response to https://github.com/r-lib/vctrs/issues/1780.