From mtmorgan at fhcrc.org  Fri Apr  6 10:55:50 2007
From: mtmorgan at fhcrc.org (Martin Morgan)
Date: Fri, 06 Apr 2007 01:55:50 -0700
Subject: [Rd] Problem with S4 inheritance: unexpected re-initialization?
In-Reply-To: <46152B35.2070200@aon.at> (cstrato@aon.at's message of "Thu, 05
	Apr 2007 19:00:37 +0200")
References: <46112187.3090009@aon.at> <4612CCAB.1080809@fhcrc.org>
	<4613BE1D.5000507@aon.at> <4613F186.1060507@fhcrc.org>
	<46152B35.2070200@aon.at>
Message-ID: <6phfy7dlw4p.fsf@gopher4.fhcrc.org>

Christian, Herve --

In the end, I think you're being caught by partial matching in
function arguments:

> f <- function(x1) cat("x1:", x1, "\n")
> f(x="hello")
x1: hello 

In the functional call x gets matched to x1. In your initialization
method for SubSubClassB2, nameB gets matched to nameB2 and so not
passed as ... in the callNextMethod.

The apparently extra calls to initialize with the wrong arguments come
about because of the validity methods -- validity gets checked by
coercing derived objects to their superclass, which involves a call to
'new' (hence initialize) followed by copying appropriate slots.

The funny effect where class(object) seems to trigger construction of
a new object is lazy evaluation -- the 'object' argument to
setValidity is not evaluated until needed, i.e., until class(object)
(anything would trigger this, including force(object)); only then do
you see the attempt to create the new object of the previous
paragraph.

Without partial matching issues, the use of callNextMethod as here:

setMethod("initialize", "SubSubClassB2",
   function(.Object, nameB2="MyNameB2", ...) {
      if (nameB2 == "") nameB2 <- "DefaultNameB2";
      callNextMethod(.Object, nameB2=nameB2, ...)
   }
)

is fine (though weird to have a prototype, a named argument, and a
conditional assignment!).

With

  setMethod("initialize", "SubSubClassB2",
    function(.Object, ...) {
       .Object <- callNextMethod()
       if (.Object at nameB2 == "") .Object at nameB2 <- "DefaultNameB2"
       .Object
    }
  )

as suggested by Herve, the "SubClassB" initializer matches the nameB
argument to 'new' to the nameB argument of the initialize method for
SubClassB. 

Simplifying the code to illustrate the problem was very helpful. It
would have been better to not present the validity methods (this might
have been a different thread), to remove the cat statements, to remove
unnecessary slots and classes (SubSubClassB1) and to name classes,
slots, and argument values in an easier to remember way (e.g., classes
A, B, C, slots a, b, c)

Hope that helps. 


Martin


cstrato <cstrato at aon.at> writes:

> Dear Herve
>
> Thank you once again for taking your time, I appreciate it very much.
> I followed your advice and kept only the minimum code, which I believe
> is necessary. I have even removed SubClassA, so the inheritance tree is:
>
>    BaseClass <- SubClassB <- SubSubClassB1
>              <- SubClassB <- SubSubClassB2
>
> The source code is shown below and the examples are:
>  > subsubB1 <- new("SubSubClassB1", filename="MyFileNameB1", 
> nameB="MyNameB")
>  > subsubB2 <- new("SubSubClassB2", filename="MyFileNameB2", 
> nameB="MyNameB")
>
> Although SubSubClassB1 and SubSubClassB2 differ only slightly, the results
> for "subsubB1" are correct, while "subsubB2" gives a wrong result, see:
>  > subsubB2 <- new("SubSubClassB2", filename="MyFileNameB2", 
> nameB="MyNameB")
>  > subsubB2
> An object of class "SubSubClassB2"
> Slot "nameB2":
> [1] "MyNameB"
>
> Slot "nameB":
> [1] ""
>
> Slot "filename":
> [1] "MyFileNameB2"
>
> Only, when adding "nameB2="MyNameB2", I get the correct result, see:
>  > subsubB2 <- new("SubSubClassB2", filename="MyFileNameB2", 
> nameB="MyNameB",nameB2="MyNameB2")
>  > subsubB2
> An object of class "SubSubClassB2"
> Slot "nameB2":
> [1] "MyNameB2"
>
> Slot "nameB":
> [1] "MyNameB"
>
> Slot "filename":
> [1] "MyFileNameB2"
>
> It is not clear to me why omitting "nameB2" results in the wrong assignemt
> of the value for "nameB" to "nameB2"?
>
> When running both examples, the sequence of initialization and validation
> method is also completely different.
> For "SubSubClassB1" I get the correct and expected sequence of events:
>  > subsubB1 <- new("SubSubClassB1", filename="MyFileNameB1", 
> nameB="MyNameB")
> ------initialize:SubSubClassB1
> ------initialize:SubClassB
> ------initialize:BaseClass
> ------setValidity:BaseClass
> ------setValidity:SubClassB
> ------setValidity:SubSubClassB1
>
> In contrast, for "SubSubClassB2" I get the following sequence of events:
>  > subsubB2 <- new("SubSubClassB2", filename="MyFileNameB2", 
> nameB="MyNameB")
> ------initialize:SubSubClassB2
> ------initialize:SubClassB
> ------initialize:BaseClass
> ------setValidity:BaseClass
> ------initialize:SubClassB
> ------initialize:BaseClass
> ------setValidity:BaseClass
> ------setValidity:SubClassB
> ------setValidity:SubClassB
> ------initialize:SubClassB
> ------initialize:BaseClass
> ------setValidity:BaseClass
> ------setValidity:SubClassB
> ------setValidity:SubSubClassB2
>
> Furthermore, the slot "filename" is first initialized correctly to
> "filename=MyFileNameB2", but then it is twice initialized incorrectly
> to "filename=ERROR_FileName", indicating that it may not have been
> initialized at all. I do not understand this behavior, why is this so?
>
> I really hope that this time the code below is acceptable to you.
> Thank you for your help.
> Best regards
> Christian
>
> # - - - - - - - - - - - - - - - - BEGIN - - - - - - - - - - - - - - - - 
> - - - -
> setClass("BaseClass",
>    representation(filename = "character", "VIRTUAL"),
>    prototype(filename = "DefaultFileName")
> )
>
> setClass("SubClassB",
>    representation(nameB = "character"),
>    contains=c("BaseClass"),
>    prototype(nameB = "NameB")
> )
>
> setClass("SubSubClassB1",
>    contains=c("SubClassB")
> )
>
> setClass("SubSubClassB2",
>    representation(nameB2 = "character"),
>    contains=c("SubClassB"),
>    prototype(nameB2 = "NameB2")
> )
>
> # - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
> setMethod("initialize", "BaseClass",
>    function(.Object, filename="",  ...) {
>       cat("------initialize:BaseClass------\n")
>       cat("BaseClass:init:class(.Object) = ", class(.Object), "\n", sep="")
>       if (filename == "" || nchar(filename) == 0) filename <- 
> "ERROR_FileName"
>       cat("BaseClass:init:filename = ", filename, "\n", sep="")
>       .Object <- callNextMethod(.Object, filename=filename, ...)
>       .Object at filename <- filename
>       .Object
>    }
> )
>
> setValidity("BaseClass",
>    function(object) {
>       cat("------setValidity:BaseClass------\n")
>       cat("BaseClass:val:class(.Object) = ", class(object), "\n", sep="")
>       msg <- NULL
>       if (!(is.character(object at filename)))
>          msg <- cat("missing filename\n")
>       cat("BaseClass:val:filename = ",object at filename, "\n", sep="")
>       if (is.null(msg)) TRUE else msg
>    }
> )
>
> # - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
> setMethod("initialize", "SubClassB",
>    function(.Object, nameB="",  ...) {
>       cat("------initialize:SubClassB------\n")
>       cat("SubClassB:init:class(.Object) = ", class(.Object), "\n", sep="")
>       .Object <- callNextMethod(.Object, nameB=nameB, ...)
>       .Object at nameB = nameB
>       .Object
>    }
> )
>
> setValidity("SubClassB",
>    function(object) {
>       cat("------setValidity:SubClassB------\n")
>       cat("SubClassB:val:class(object) = ", class(object), "\n", sep="")
>       msg <- NULL
>       if (!(is.character(object at nameB) && length(object at nameB) > 0))
>          msg <- cat("missing nameB\n")
>       cat("SubClassB:val:nameB = ",object at nameB, "\n", sep="")
>       if (is.null(msg)) TRUE else msg
>    }
> )
>
> # - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
> setMethod("initialize", "SubSubClassB1",
>    function(.Object, ...) {
>       cat("------initialize:SubSubClassB1------\n")
>       cat("SubSubClassB1:init:class(.Object) = ", class(.Object), "\n", 
> sep="")
>       .Object <- callNextMethod(.Object, ...)
>       .Object
>    }
> )
>
> setValidity("SubSubClassB1",
>    function(object) {
>       cat("------setValidity:SubSubClassB1------\n")
>       cat("SubSubClassB1:val:class(object) = ", class(object), "\n", sep="")
>       return(TRUE)
>    }
> )
>
> # - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
> setMethod("initialize", "SubSubClassB2",
>    function(.Object, nameB2="MyNameB2", ...) {
>       cat("------initialize:SubSubClassB2------\n")
>       cat("SubSubClassB2:init:class(.Object) = ", class(.Object), "\n", 
> sep="")
>       if (nameB2 == "") nameB2 <- "DefaultNameB2";
>       cat("SubSubClassB2:init:nameB2 = ", nameB2, "\n", sep="")
>       .Object <- callNextMethod(.Object, nameB2=nameB2, ...)
>       .Object at nameB2 <- nameB2
>       .Object
>    }
> )
>
> setValidity("SubSubClassB2",
>    function(object) {
>       cat("------setValidity:SubSubClassB2------\n")
>       cat("SubSubClassB2:val:class(object) = ", class(object), "\n", sep="")
>       msg <- NULL;
>       if (!(is.character(object at nameB2) && length(object at nameB2) > 0))
>          msg <- cat("missing nameB2\n")
>       cat("SubClassB:val:nameB2 = ",object at nameB2, "\n", sep="")
>       if (is.null(msg)) TRUE else msg
>    }
> )
>
> # - - - - - - - - - - - - - - - END - - - - - - - - - - - - - - - - -
>
>
> Herve Pages wrote:
>> Hi Christian,
>>
>> cstrato wrote:
>>   
>>> Dear Herve
>>>
>>> Thank you for your helpful comments, and I especially appreciate that
>>> you tried to run my package. I will try to answer each point separately.
>>>
>>> Herve Pages wrote:
>>>     
>>>> Hi Christian,
>>>>
>>>> I can only give you a few reasons why IMO it is very unlikely that
>>>> anybody
>>>> will be able to help you on this, with the current form of your post.
>>>>
>>>> 1) Unless you have a really good reason to do so, don't attach a package
>>>>    to your post. Do your best to provide a few lines of code that anybody
>>>>    can easily copy and paste into their session.
>>>>   
>>>>       
>>> Sorrowly, sometimes, a few lines of code are not sufficient to show
>>> the problem. Furthermore, most of the time there are complaints that
>>> people do not provide enough information, an issue I wanted to avoid.
>>>     
>>
>> The code you provide below is still too long and overcluttered with stuff that is
>> probably unrelated with the issue you want to discuss. Your class definitions
>> still have slots that we don't care about. Basically if you want to
>> discuss an S4 issue, you should get rid of all this file system related stuff
>> (the 'dirfile', 'filedir', 'filename' slots, the 'pathFile' function, the dozens
>> of calls to 'basename', 'dirname', 'getwd', 'file.dir' etc...)
>>
>> Also your code is dirty and hard to read. Take this for example:
>>
>>   "initialize.BaseClass" <-
>>   function(.Object, filename=character(), filedir=as.character(getwd()), ...) {
>>     print("------initialize:BaseClass------")
>>   print(paste("BaseClass:init:class(.Object) = ", class(.Object)))
>>
>>   #   .Object <- callNextMethod(.Object, ...);
>>
>>     dirfile <- pathFile(filename, filedir);
>>   print(paste("BaseClass:init:dirfile = ", dirfile))
>>
>>     .Object <- callNextMethod(.Object, filename=filename, filedir=filedir, ...);
>>
>>     .Object at filename <- filename;
>>     .Object at filedir  <- filedir;
>>     .Object;
>>   }#initialize.BaseClass
>>
>>   setMethod("initialize", "BaseClass", initialize.BaseClass);
>>
>> o It's not properly indented.
>> o Why those empty lines in the middle of such a short function?
>> o Why those semi-columns at the end of lines?
>> o Why put the implementation of the initialize method into a separate function?
>> o Why use this construct 'print(paste())' instead of just 'cat()'?
>> o Why leave the first call to callNextMethod() commented?
>> o What do you do with 'dirfile', except that you print it? Do you need this for
>>   the purpose of the S4 discussion?
>>
>> What about adopting this style:
>>
>>     setMethod("initialize", "BaseClass",
>>         function(.Object, filename=character(), filedir=as.character(getwd()), ...)
>>         {
>>             cat("------initialize:BaseClass------\n")
>>             cat("BaseClass:init:class(.Object) = ", class(.Object))
>>             .Object <- callNextMethod(.Object, filename=filename, filedir=filedir, ...)
>>             .Object at filename <- filename
>>             .Object at filedir  <- filedir
>>             .Object
>>         }
>>     )
>>
>> That's for the style. Now let's talk about the semantic. Here is the definition of
>> your BaseClass class:
>>
>>     setClass("BaseClass",
>>         representation(
>>             "VIRTUAL",
>>             filename="character",
>>             filedir="character"
>>         ),
>>         prototype(
>>             filename = character(),
>>             filedir  = as.character(getwd())
>>         )
>>     )
>>
>> (Note that this definition is what _I_ get after I cleaned it and indented it which
>> is something that _you_ are expected to do.)
>>
>> o Initializing the 'filename' slot to character() is useless since this is the default.
>> o Wrapping getwd() inside as.character() is useless since getwd() returns a character vector.
>> o BUT MOST IMPORTANTLY THAN ANYTHING ELSE: given the fact that you've provided a prototype
>>   for class BaseClass, your initialize method is useless since it does _nothing_ more
>>   than what the default initialize method does! If you want to define your own "initialize"
>>   method for the only purpose of printing messages, then you could do it in a much simpler
>>   way:
>>
>>     setMethod("initialize", "BaseClass",
>>         function(.Object, ...)
>>         {
>>             cat("------initialize:BaseClass------\n")
>>             cat("BaseClass:init:class(.Object) = ", class(.Object), "\n", sep="")
>>             callNextMethod()
>>         }
>>     )
>>
>> But as I said initially, the file system related stuff is useless to illustrate the
>> S4 issue you want to show us so why didn't you just provide:
>>
>>     setClass("BaseClass",
>>         representation("VIRTUAL", slot1="character"),
>>         prototype(slot1="aaa")
>>     )
>>
>>     setMethod("initialize", "BaseClass",
>>         function(.Object, ...)
>>         {
>>             cat("------initialize:BaseClass------\n")
>>             cat("BaseClass:init:class(.Object) = ", class(.Object), "\n", sep="")
>>             callNextMethod()
>>         }
>>     )
>>
>> See? I ended up by replacing 25 lines of your hard-to-read/confusing code by 12
>> lines of clean code containing only the STRICTLY NECESSARY stuff for an S4 discussion.
>> That's what you should have done instead of sending us 150 lines of code.
>>
>> Sorrowly people will not try to read your code unless it's obvious that _you_ did your
>> best to provide the cleanest/simplest possible code that illustrates the issue you want
>> to discuss.
>>
>> Also maybe you could try to read a little bit more about S4 and initialization mechanisms.
>>
>> Cheers,
>> H.
>>
>>
>
> ______________________________________________
> R-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel

-- 
Martin Morgan
Bioconductor / Computational Biology
http://bioconductor.org



[Rd] Problem with S4 inheritance: unexpected re-initialization?
Martin Morgan