hansenlab / minfi

Devel repository for minfi
58 stars 68 forks source link

RGChannelSet change from eSet subclass to SummarizedExperiment subclass #125

Closed alanocallaghan closed 6 years ago

alanocallaghan commented 6 years ago

Hi, I note a discrepancy between the package documentation and the code, notably below RGChannelSet inherits from SummarizedExperiment: https://github.com/kasperdanielhansen/minfi/blob/master/R/rgset.R#L3

Yet the documentation makes no reference to this class and in fact claims that RGChannelSet inherits from eSet (which used to be true). https://github.com/kasperdanielhansen/minfi/blob/master/man/RGChannelSet-class.Rd#L125

I would note that changing the inheritance of established classes is a pretty unpleasant thing to do with published code in any case, but please update the documentation to reflect the changes made. Thanks.

kasperdanielhansen commented 6 years ago

Thanks, I'll fix this.

If it is any consolation, the change was not done lightly. This change is paving the way for dramatically reducing the memory footprint of minfi; the goal is to be able to run minfi in bounded memory with data loaded on demand from disk.

Best, Kasper

On Mon, Dec 11, 2017 at 10:44 AM, Alanocallaghan notifications@github.com wrote:

Hi, I note a discrepancy between the package documentation and the code, notably below RGChannelSet inherits from SummarizedExperiment: https://github.com/kasperdanielhansen/minfi/blob/master/R/rgset.R#L3

Yet the documentation makes no reference to this class and in fact claims that RGChannelSet inherits from eSet (which used to be true). https://github.com/kasperdanielhansen/minfi/blob/ master/man/RGChannelSet-class.Rd#L125

I would note that changing the inheritance of established classes is a pretty unpleasant thing to do with published code in any case, but please update the documentation to reflect the changes made. Thanks.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kasperdanielhansen/minfi/issues/125, or mute the thread https://github.com/notifications/unsubscribe-auth/AEuhn9-hlB-5hFPoqOszwvuvShLoZT8uks5s_U3vgaJpZM4Q9mJr .

kasperdanielhansen commented 6 years ago

Btw, since the accessor functions still exists, you should be able to use existing code without changes. You can update old objects using updateObject().

alanocallaghan commented 6 years ago

Sounds useful, especially for 850k data (currently only feasible for high memory machines).

The issues I was having relate to accessing and changing red/green or methylated/unmethylated values in MethylSet and RGChannelSet objects. eSet objects use assayDataElement while SummarizedExperiment uses the assay function. The change in inheritance broke my code, with some pretty confusing error messages, since the package docs didn't make the change clear

Thanks!