rformassspectrometry / Spectra

Low level infrastructure to handle MS spectra
https://rformassspectrometry.github.io/Spectra/
34 stars 24 forks source link

`Spectra` accessor methods: which variables get a replace method (why)? #176

Closed meowcat closed 3 years ago

meowcat commented 3 years ago

Hi,

is there a specific reason or a rule for why some spectraVariables have a replace method and some others don't? For example, isolationWindowLowerMz has a <- method but precScanNum doesn't.

Interestingly, I have not yet seen reason to change the isolationWindowLowerMz, but I have had a reason to change the precScanNum (there were issues with Thermo Tribrid converted mzMLs in polarity switching mode where precursor scan was incorrectly assigned). But I can always just set the spectraVariables directly, so this isn't the problem; I just wonder why one has a replace method and the other doesn't.

Ciao -M.

jorainer commented 3 years ago

good point. there was no special reason for that. we mostly added setter method if we also had one in the MSnbase package. as you said, for all variables it is possible to set the variable with $<- so I would only implement new dedicated setter method were it is really needed (or has an advantage over the generic $<-) - otherwise we just have tons of different methods. I'd rather try to keep the package small and tidy.

meowcat commented 3 years ago

OK, I only stumbled upon it because I was being lazy and doing


for(spectra_alias in .spectra_aliases_list) {

  #' @rdname hidden_aliases
  setMethod(spectra_alias, "MsBackendMapping", .alias_read_fun(spectra_alias))

  # Note here we need to check if this is a read-only attribute
  if(!(spectra_alias %in% .spectra_aliases_ro))
  {
    #' @rdname hidden_aliases
    setReplaceMethod(spectra_alias, "MsBackendMapping", .alias_write_fun(spectra_alias))
  }
}

where .alias_read_fun, .alias_write_fun return the typed failsafe accessor/replacer. I assume this isn't really best practice when it comes to S4 classes :)