search

rformassspectrometry / Spectra

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

Add filterPrecursorMzValue to support filtering by multiple m/z values #232

Closed jorainer closed 2 years ago

jorainer commented 2 years ago

This PR adds the filterPrecursorMzValue function discussed in issue #230

lgatto commented 2 years ago

Code-wise, looks good to me, but I am wondering whether we aren't making things too complicated with that many functions. Wouldn't it be easier to have a single vectorised filterPrecursorMz() function? Same applies to other cases.

jorainer commented 2 years ago

Yes, we're adding yet another filter function, but IMHO it is justified here, since the original filterPrecursorMz and the newly added filterPrecursorMzValues work differently, the former based on m/z ranges, the latter on individual m/z values. This is similar to the discussion in #133. I believe it is easier on the user to have dedicated functions instead of having to set multiple additional parameters. Also, changing filterPrecursorMz to a vectorized version might break backward compatibility (also based on what users are used from the MSnbase package).

But obviously that's open for discussion @lgatto @sgibb

lgatto commented 2 years ago

Ok, indeed, thanks. I totally agree that different appropriately named functions are better that many arguments.

What about renaming filterPrecursorMz() to filterPrecursorMzRange() to clarify that it works on ranges, rather than values, and be consistent with #133 ?

And re vectorisation, it's easy enough to pipe multiple filterPrecursorMz[Range]() into each other to filter multiple ranges.

jorainer commented 2 years ago

I've now added a filterPrecursorMzRange method and deprecated the filterPrecursorMz method - I wouldn't like to remove it yet, because we have many workflows depending on that method.