@AnnaWiniwarter , in this case I must say the error is expected for me. If it should do something other than raise an error when appending measurements with spectra of different shapes, we have to decide what. What do you think it should do?

I wonder if you could make the error message more explicit for this case? I.e. specify that it can't append to MSSpectroMeasurements if the mass range (does it also apply to a different mass resolution?) is different. Would that be possible?

I am in middle of the review. Should be done with it tomorrow, Thursday latest.

Regarding the appending error. I also think it could be more explicit, if it really is not possible to add them together, because of logical scientific reasons. Although, they are not clear to me right now. Why different amount of spectra couldn't be added together?

• If they have the same AMU range, is it then like doing "2 + 3 = 5"? Meaning a list with 2 spectra plus a list with 3 spectra.
• If they don't, then plotting them together with MS plot should be fine, since the spectra match the breaks in the MS plot. So what is preventing it?

@matenestor ,
Great! I will wait on your review :)
To answer, in the two cases you present:

• Appending spectrum_series with different numbers of spectra should be no problem, like you said. If an error came up then, it would definitely be a bug to fix.
• If the AMU range is different, you get the error Anna describes. Good point that it should be possible to plot in a sensible way. The problem is the spectrum_series data structure - this is not just a list of spectrums, but a 2-D data series. It can only represent spectra with the same number of AMU points because a numpy array has to be rectangular. Which leaves the following options. (Keep in mind that a lot of analysis tools are likely to come on top of the data structure that we choose.)
  • ditch SpectrumSeries and just allow an MSMeasurement to have a list of associated Spectrum objects. Then each spectrum object can have its own x series (AMU range), so you would gain flexibility on that front. What you loose is the ability to do vectorized analysis of the spectra - that might not matter for the cases so far, but if someone is taking continuous spectra overnight, they would feel the difference between treating the data as a 2-D array vs looping over 1-D arrays. For other techniques with faster spectrum acquisition times, like UV-Vis, this difference is more critical. (On a related idea for simplification, maybe we can get rid of most or all of the SpectroMeasurement classes, as suggested by Kenneth's comment here.)
  • allow for lists of SpectrumSeries when the x series is not constant. The problem here is that it makes all the methods acting on the spectra much less intuitive - you'd always have to specify which SpectrumSeries you are acting on.
  • do some kind of manipulation when appending SpectrumSeries objects to get them on the same xseries. That could be interpolating the spectra with the larger x range onto the smaller xseries or filling the smaller spectra with 0's or None's to cover the larger xseries. The disadvantage is that it might not be intuitive or transparent, and you wouldn't get the plot that you describe with the AMU range shown clearly in the range of the spectra.
  • Raise an error message when SpectrumSeries with different xseries are added together, but point the user towards a method (like cast_onto_x) that can be used on a SpectrumSeries or SpectroMeasurement to explicitly change the x range of the spectra. This method could take inputs allowing for the above options, and could explicitly and transparently get spectra into the right shape for appending.

Let me know what you think. And I look forward to your review! If you can catch why available spectra are not always loaded by default, as Anna noticed, that's for sure something I hope to fix.

I am still getting an error when combining an EC measurement with spectra. I don't think I have found your suggestion, but what it should call would probably be ECMeasurement class and then it would combine it with SpectroMeasurement? That really smells like another ECSpectroMeasurement class, though 😄

Hi @matenestor , I have implemented your review! :)
I didn't do anything, though, on the issue of appending spectra of different m/z-spans. I think we have to deal with that seperately, becase we (I at least) don't know what exactly ixdat should do in that case (and I think the existing error message is clear enough).
I also didn't do anything about your last comment... you want to combine an ECMeasurement with a MSSpectrumSeries without any MID data? That might be do-able with just an extra line in TECHNIQUE_CLASSES in techniques.__init__:

"EC-MS_spectra": SpectroECMeasurement

Can you give it a try? :)

@ScottSoren I replied to your comments and I consider them mostly resolved. Some are left open for you to read, so know about it.

And the rest is waiting for your answer (those are the left out comments from the first round). There is three of them. Once you comment, I will approve the PR.

@matenestor , I've commented on the remaining issues!
@AnnaWiniwarter , I've made that error message more explicit.
Forgive the long period of inactivity. Hopefully it is good to merge now :)