search

waldronlab / MultiAssayExperiment

Bioconductor package for management of multi-assay data
https://waldronlab.io/MultiAssayExperiment/
70 stars 32 forks source link

colData<- and sampleMap<- validity #271

Closed vobencha closed 4 years ago

vobencha commented 4 years ago

Hi,

It looks like sampleMap<- and colData<- replacement methods for the MAE don't call validObject() or MultiAssayExperiment:::.harmonize() so the user can perform quite a few operations before they realize they have a broken object.

These return a MAE with no error but the object is not valid. The first example of the empty DataFrame() is on the man page.

No error > sampleMap(mae) <- DataFrame() 

but the object isn't valid

> validObject(mae)
Error: subscript contains invalid names

No error > sampleMap(mae) <- DataFrame(assay="testAssay", primary="testPrimary", colname="testColname")

but the object isn't invalid

> validObject(mae)
Error in validObject(mae) : 
  invalid class “MultiAssayExperiment” object: 1: 
    All samples in the 'sampleMap' must be in the 'colData'
invalid class “MultiAssayExperiment” object: 2: 
    'sampleMap' assay column not a factor

The same is true for colData<-. I can even change the rownames on the MAE colData to bogus names (i.e., don't match sampleMap) and no error is thrown.

An error is thrown when the user attempts single bracket subsetting but this seems a little late and the validity check should really be at the level of the setter.

Val

lwaldron commented 4 years ago

Thank you for reporting these @vobencha!

vobencha commented 4 years ago

Thanks for the fix @LiNk-NY @lwaldron .

charles-plessy commented 4 years ago

Hi, I appreciate that this commit will enhance the quality of all the packages utilising MultiAssayExperiment objects, but ~2 weeks before the release, it broke our package (CAGEr), at a time with telework, school closures and possible confinement, I am not sure to have time find a fix in time. Would it be possible to postpone that change to after the release ?

LiNk-NY commented 4 years ago

Hi Charles, @charles-plessy I'm glad you find MultiAssayExperiment useful. Sorry for the breaking change. This issue is a validity one and I'd rather not revert the change.

Bioconductor can make accommodations to allow you to patch the package in the release branch after the release. Feel free to reach out to me for help regarding the change. Be well and stay safe.