Open meowcat opened 5 years ago
I would opt for peak indices (or even peak IDs/names). Using numeric
m/z to identify peaks can be problematic, because comparison of double data types depends on the precision of the system/representation of double.
To add peaks to a spectrum I would define a dedicated addPeaks
method. And I am wondering if you would really allow $mz <-
assignments, or if you would throw an error when that is called.
While $mz <-
and $intensity
sounds nice to have, providing a dedicated API with methods addPeaks
, filterPeaks
etc might be easier for the user to understand what is going on - but that's just my opinion.
Definitely indices, to avoid float comparisons.
As @jorainer says, an API becomes mandatory if we need to maintain object validity depending on operations.
Hence my suggestion to keep things as simple as possible now, and compute values on-the-fly. If we hit a bottleneck, we'll reconsider. Premature optimisation is the root of all evil. And so is feature creep.
And I am wondering if you would really allow $mz <- assignments, or if you would throw an error when that is called.
My suggestion here was to allow $<-
for modifying m/z or intensity (say, recalibrating) but not for adding or removing peaks.
feature creep
You are completely right. We should keep it simple.
As we have discussed already, modifying
mz
andintensity
can create issues with mapping spectral annotations. There are two options for mapping annotations - by peak indices or by m/z; the former will fail when subsetting (e.g. filtering) the peaks, the latter will fail when changing masses e.g. recalibrating.We could get around this if we impose the following restrictions:
mz
orintensity
is changed, the length of the replacement must be equal to the original vector, i.e. changingmz
andintensity
never changes the mappingpeakId
accessor or field is added. Any subsetting of the spectrum must happen by replacingpeakId
.E.g.
There is an issue with this, which is that I currently don't know how we could add peaks to the spectrum. This is an operation I definitely do. A solution would be to allow some kind of specBind or merge operation.