CHANGES IN VERSION 0.6 ----------------------- SIGNIFICANT USER-VISIBLE CHANGES o rSFFreader is designed to mimic the Illumina interface used in ShortReads specifically SffReads mimics very similarly to ShortRead and SffReadsQ mimics ShortReadQ, at this all functions and methods available in ShortRead for ShortRead and ShortReadQ are available in SffReads and SffReadsQ NEW FEATURES o All features are new BUG FIXES Comments by Herve, during the Bioconductor submission 1. 'R CMD check' produces the following warning: * checking for missing documentation entries ... WARNING Undocumented S4 methods: generic 'hist' and siglist 'SffReads' I think this is because of the space after the comma in this alias (in SffReads-class.Rd): \alias{hist, SffReads-method} Note that you have other aliases with an extra space that would need to be removed: \alias{sread, ANY-method} \alias{id, SffReads-method} \alias{name, SffReads-method} \alias{append, SffReads-method} \alias{width, SffReads-method} \alias{writeFasta, SffReads-method} \alias{hist, SffReads-method} ##### MS: I got rid of the spaces in all of these. I'm not sure what to do with the other comments. ##### I think you don't get a warning for those because you are not exporting those methods. The fact that they work despite not being exported is because of some glitch with how NAMESPACEs are currently implemented but I would argue that you should explicitely export all the methods you define in your package. Also note that sread() is a regular function so the above alias doesn't make sense. 2. Because you define a "hist" method and the hist() function belongs to the graphics package you need to Imports graphics and the import hist from graphics in your NAMESPACE. But do you really need to define such a method, just to have the user type 'hist(sff)' instead of 'hist(width(sff))'? ############ MS: Agreed, removed the hist function from rSFFreader ############ 3. Your sread() function is masking the sread() function defined in the ShortRead package. This should be avoided as loading rSFFreader will break ShortRead::sread(). I encourage you to talk with Martin for the best way to deal with this (e.g. maybe ShortRead::sread() should be made generic so you could define a method for SffReads objects). ########### MS: Defining sread as a generic would be ideal, but I assume Martin had a reason not to do so and I'd be happy to talk to him about that. However, masking the sread does not actually break the ShortRead::sread() while the ShortRead function is not called functionally my function will return the same result. ########### 4. 3 man pages lack examples (even a trivial one): SffHeader-class.Rd, load454SampleData.Rd, and loadIonSampleData.Rd. ########## MS: Added examples for SffHeader-class.Rd, load454SampleData and loadIonSampleData. Both examples are moderately in-depth. ######### 5. The standard sentence that we see in a lot of man pages: Objects can be created by calls of the form ‘new("SffHeader", ...)’. is confusing the user because, most of the time, this is not how the user should create SffHeader objects. Please remove in all man pages and focus the documentation on how the user will really create those objects (e.g. with a constructor or a read* function). ######### MS: Had the text on how "users will really create those object" already in there and went ahead and removed any reference to using "new". ######### 6. In SffHeader-class.Rd please explain a little bit more the structure of the object returned by header(). Not just a list but a list of list. What are the top-level elements of the outter list? What are the element of the inner list? Are the list named? Etc... It would help to clarify if you could illustrate with an example involving more that 1 file passed to readsffheader(). ######## SH: Added a much more in-depth description including a description of each element in the list of meta-data. Also added a much more comprehensive set of examples showing how to access header information both when using readSffHeader, and when using readSff. Both are carried out with a list of sffFiles to illustrate how the object works when more than 1 file is passed to readSffHeader() or readSff(). ######## 7. Many methods for SffReads objects are under-documented in the man pages: adapterClip, adapterClip<-, clipMode, clipMode<-, customClip, customClip<-, fullClip, qualityClip, qualityClip<-, rawClip, id, writeFasta. Also the availableClipModes, method is not documented at all (there is an alias for it but the method is not mentioned at all in the man page). All those methods would need to be illustrated with at least 1 example (some of them are introduced in the vignette though but they need to be fully documented in the man pages). ########### SH: adapterClip : see availableClipModes.Rd adapterClip<- : see availableClipModes.Rd clipMode : see availableClipModes.Rd clipMode<- : see availableClipModes.Rd customClip : see availableClipModes.Rd customClip<- : see availableClipModes.Rd fullClip : see availableClipModes.Rd qualityClip : see availableClipModes.Rd qualityClip<- : see availableClipModes.Rd rawClip : see availableClipModes.Rd rawClip<- : see availableClipModes.Rd id : There was a small typo in the description. This isn't a slot, just a helper function which is included to maintain compatibility with ShortRead. The description has been updated to reflect this. writeFasta : also a small typo, this is a wrapper for the writeFasta function in ShortRead (previously was listed as writeFASTA) availableClipModes : added a new help file with an extensive set of examples showing how clipping works, an example of how to set/get clip points is provided, as is a description of why maintaining clipping information is useful. ############## 8. SffReads() is documented as if it was a generic with methods when it's actually implemented as a regular function. Also the man page should make it clear that this is a constructor, and put it in a dedicated section (Constructor section) instead of putting it in the Methods section where methods that operate on an SffReads object are described. Some examples of how to use this constructor would need to be provided. ########## MS: Fixed the documentation, moved to a contstructor section ########## 9. Some man pages (e.g. SffReads-class.Rd) and .R files (e.g. methods-Misc.R) use IRange instead of IRanges. ######### MS: Fixed ######### 10. Function naming: why not readSff (or readSFF), readSffGeometry (or readSFFgeometry) and readSffheader (or readSFFheader) instead of readsff, readsffgeometry and readsffheader? ######### MS: Why not? Sure, I changed them ######### 11. Why have readsffgeometry() return a 2-elt list with the 1st elt (nReads) being the length of the 2nd (Read_Widths)? This is redundant and you could just return a single integer vector containing the read widths. Also in the \example section of the man page you have this line: sffstats <- readsffgeometry(...) Why not use more consistent naming e.g. sffgeom <- readsffgeometry(...) or if readsffgeometry() is really to be considered a function returning stats then it could be renamed: sffstats <- readsffstats(...) ################ MS: The read SFF geometry function is a direct return from C where it was simpler to have it be two element, number of reads to iterate over and the read lengths in C. I return the result to R, in case someone wishes to know the geometry of the file, however I expect this function is rarely expected to be used. I see no major reason not to be a little redundant here, since a fix would be to return only the second element. Will change if I must. MS: Fixed the Rd file, to be sffgeom ################# ################ 12. C code: - Register .Call entry points. #### MS: I did, in the R_init_rSFFreader.c, didn't I? #### - Why use malloc here since the array has a fixed size: uint8_t *byte_padding = (uint8_t*) malloc(sizeof(uint8_t)*(8)); This leads to a memory leak (even small) because the memory is allocated each time readSFF() is called but is never freed. The standard way to allocate this array is to do: uint8_t byte_padding[8]; or: static uint8_t byte_padding[8]; With 'static', the array is allocated once for all at load time and reused each time readSFF() is called so it's like a global variable but its scope is limited to the body of readSFF(). It's also automatically initialized with zeroes. Without 'static', a new array is allocated each time readSFF() is called and freed when readSFF() returns. It's not initialized so each time it contains random garbbage. ######## MS: Turns out this is probably legacy code and the variable no longer used, removed it. ######## - A more serious memory leak is in count_reads_sum(): for(i=0; i