search

pneuvial / c3co

Inferring cancer cell clonality from copy-number data
5 stars 1 forks source link

S4: What was the reason/rationale for using S4? #30

Open HenrikBengtsson opened 6 years ago

HenrikBengtsson commented 6 years ago

I probably asked before but I don't remember the answer; what's the reason for using the S4 class system (rather than S3)? It don't see anything that is making use of S4-unique features.

pneuvial commented 6 years ago

Initially there were no classes at all, and Julien suggested to define objects and methods for plotting/formatting. I don't remember why we chose S4 and not S3.

HenrikBengtsson commented 6 years ago

Ok. I agree the use of classes/methods is very useful here. Disclaimer: I'm not a major fan of S4 (although I was one of the first people in the world waiting for it/and trying it out). (*) Clarification 2018-03-03: I understand why S4 exists and what it tries to achieve, but in many cases it is overkill and S3 works perfectly well.

Are there any objections if I go ahead and add S3 class attributes to some of the data structures that are currently just lists (and lists of lists)? For instance, what's currently returned by segmentData() and it's components.

jchiquet commented 6 years ago

Yep, totally agree : S4 is a real pain... I personally like R6 (or refClass) much for more OOP formalism, but I am not sure it is relevant for c3co. It probably depends if some objets / structures are common with joinSeg.

Anyway, S3 sounds good to me.

pneuvial commented 6 years ago

Currently there is no class/methods in jointSeg, so not much to share yet!

pneuvial commented 6 years ago

and thanks @HenrikBengtsson for offering to S3-ify this.

HenrikBengtsson commented 6 years ago

I'll add S3 in c3co, but some of it should probably move up to jointseg at some point.

Don't think there's a need for traditional, mutable, class-oriented OOP objects. I'll keep it simple and functional as far as possible. (author of R.oo so I think I can appreciate your preferences) BTW, refClasses is S4-based so it brings in that extra runtime overhead we see there.