Extension of bruker precursor information (2nd attempt) #3191

xjasg · 2024-10-10T20:39:57Z

Extension of the analysis of Bruker files (precursor information) based on the timsconvert Python code
MS_inverse_reduced_ion_mobility, MS_collisional_cross_sectional_area, MS_peak_intensity.
lowest observed mz and highest observed mz included

chambm

Looks much better! Still a bit of work to do though.

chambm · 2024-10-11T16:45:34Z

pwiz/data/msdata/Serializer_MGF.cpp

@@ -132,6 +132,16 @@ void Serializer_MGF::Impl::write(ostream& os, const MSData& msd,
                os << " " << intensityParam.valueFixedNotation();
            os << '\n';

+            CVParam inverseReduceIonMobility = scan->cvParam(MS_inverse_reduced_ion_mobility);
+            if (!inverseReduceIonMobility.empty())
+                os << "INVREION=" << inverseReduceIonMobility.valueFixedNotation();


Did you check whether some official product already writes it this way? Because according to the official MGF spec this is not a valid parameter. https://www.matrixscience.com/help/data_file_help.html
It should be ION_MOBILITY= and it has a weird format that reproduces the PEPMASS parameter for some reason (see the link). I don't see why 1/k0 needs a separate parameter if an official one already exists. Except maybe for being sure of what kind of ion mobility is being recorded.

okay, I´ll check this.

I hope it´s done now. :-)

chambm · 2024-10-11T16:46:22Z

pwiz/data/msdata/Serializer_MGF.cpp

+
+            CVParam collisionalCrossSectionalArea = scan->cvParam(MS_collisional_cross_sectional_area);
+            if (!collisionalCrossSectionalArea.empty())
+                os << "COLLCROSSSA=" << collisionalCrossSectionalArea.valueFixedNotation();


There is no official MGF parameter for this. I would be more comfortable with this in the TITLE string, unless a Bruker product is already writing MGFs this way.

okay, Is there an example of adding some parameters to the title string?

There's a titleMaker filter that allows for easy customization of the MGF title string with a user-defined format. Although it doesn't support CCS currently, only instrument-level ion mobility metrics. If you don't want to use that, you can add "ccs=" to the title string above this, before the line:
if (!scanTimeParam.empty())
os << "RTINSECONDS=" << scanTimeParam.timeInSeconds() << '\n';

Okay, that sounds good, I'll implement it that way.

Should be finished.

pwiz/data/vendor_readers/Bruker/SpectrumList_Bruker.cpp

pwiz_aux/msrc/utility/vendor_api/Bruker/TimsData.cpp

chambm · 2024-10-11T17:11:18Z

pwiz/data/msdata/Serializer_MGF.cpp

-    static unsigned int precision(T) { return 10; }
-    //  we want the numbers always to be in fixed format
-    static int floatfield(T) { return boost::spirit::karma::real_policies<T>::fmtflags::fixed; }
+template <typename T>


Revert whitespace only change.

sure, I´ll fix this.

Should be removed.

chambm · 2024-10-11T17:11:21Z

pwiz/data/msdata/Serializer_MGF.cpp

-            if (continueOnError)
-            {
+    {
+        SpectrumPtr s;


Revert whitespace only change.

I'm still seeing whitespace changes here. You know how to review the diff in GitHub? You can also review it locally if you set your diff engine to show whitespace changes.

yes, sorry, but unfortunately it's not finished yet. Yes, It's easy to see whitespace changes, but undoing them without removing other important things is a bit difficult for me. But I´m on it. Thank you.

Whitespaces should now be removed.

chambm · 2024-10-11T17:11:45Z

pwiz/data/msdata/Serializer_mzXML.cpp

@@ -535,7 +535,7 @@ void write_precursors(XMLWriter& xmlWriter, const vector<PrecursorInfo>& precurs
            attributes.add("activationMethod", it->activation);
        if (it->windowWideness != 0)
            attributes.add("windowWideness", it->windowWideness);
-
+        


Revert whitespace only changes.

pwiz_aux/msrc/utility/vendor_api/Bruker/TimsData.cpp

pwiz/data/msdata/Serializer_MGF.cpp

xjasg · 2024-10-11T18:00:13Z

Okay, thank you very much. I really appreciate that. I would try to fix the things you mentioned.

This reverts commit 8aeecd0.

chambm · 2024-10-15T15:18:30Z

pwiz/data/msdata/SpectrumList_MGF.cpp

+                                bal::trim(value);
+                                scan.cvParams.push_back(CVParam(MS_inverse_reduced_ion_mobility, value, MS_volt_second_per_square_centimeter));
+                            }
+                            else if (name == "COLLCROSSSA")


When moving CCS to the title, it can be parsed back in the name == TITLE block above.

chambm · 2024-10-15T15:27:37Z

pwiz_aux/msrc/utility/vendor_api/Bruker/TimsData.cpp

@@ -862,21 +854,16 @@ void TimsSpectrum::getIsolationData(std::vector<IsolationInfo>& isolationInfo) c
    if (HasPasefPrecursorInfo())
    {
        const auto& info = GetPasefPrecursorInfo();
-        isolationInfo.resize(1, IsolationInfo{
-	                             info.isolationMz, IsolationMode_On, info.collisionEnergy,
-	                             info.intensity


You reverted your intensity population here.

Thank you for finding this. I will fix it.

chambm

Please add ion mobility and CCS cvParams in Serializer_MGF_Test so this new code gets tested. Maybe for just one of the spectra there so it gets tested with an without.

xjasg · 2024-11-06T20:04:47Z

@chambm Thank´s for your review and comment. yes, okay, I´ll write some tests.

chambm · 2024-11-06T20:08:54Z

You don't need to add any new test, just update the existing one in Serializer_MGF_Test by adding the cvParams to one of the spectra there.

xjasg · 2024-11-06T22:30:58Z

@chambm okay.

chambm · 2024-11-12T16:12:22Z

pwiz/data/msdata/Diff.hpp

@@ -228,14 +228,17 @@ struct PWIZ_API_DECL DiffConfig : public pwiz::data::BaseDiffConfig

    bool ignoreDataProcessing;

+    bool ignoreSpectrumTitle;
+


Why is this necessary? I would expect the new CCS-in-title code to round trip the CCS so the title will be preserved and not different. What diffs were you getting without this?

No, unfortunately it is different. The serializer writes it into the title string, but the reference does not contain the text ‘ccs=...’. But this reference is used for both - reading and writing.
The other cases of code where the title is extended too seem not to run through the test, so it was not a problem there.

Sorry I dropped the ball here. What did you mean "the reference does not contain the text"?

I have to apologise. I had a lot to do in another project. As far as I understand it, the following happens: testWriteRead(const MSData& msd) uses msd as a reference. msd contains previously generated test data with a specific title. ‘serializer.write(oss, msd);’ extends the title by “ccs=...” and saves the new extended data with the new title. Then "serializer.read(iss, msd2);" reads back the written data with the new title. But after that the comparison with msd fails because of the different title. That is why I have introduced a flag that disables the comparison of the titles. Maybe its better to changed the test than the main program. I`ll try this.

I think I have now understood the problem. The additional text must be removed from the title after loading. Then everything should work. Otherwise, the title could be expanded more than once if we save and load the data twice or more. I think we would have to do the same with other title parameters.

Did you see this reply @xjasg ?

I replied, but my answers were marked as "pending".

Here is what I wrote:

"I have to apologise. I had a lot to do in another project. As far as I understand it, the following happens: testWriteRead(const MSData& msd) uses msd as a reference. msd contains previously generated test data with a specific title. ‘serializer.write(oss, msd);’ extends the title by “ccs=...” and saves the new extended data with the new title. Then "serializer.read(iss, msd2);" reads back the written data with the new title. But after that the comparison with msd fails because of the different title. That is why I have introduced a flag that disables the comparison of the titles. Maybe its better to changed the test than the main program. I`ll try this.

I think I have now understood the problem. The additional text must be removed from the title after loading. Then everything should work. Otherwise, the title could be expanded more than once if we save and load the data twice or more. I think we would have to do the same with other title parameters."

My last commit should have resolved this issue?

pwiz/data/msdata/SpectrumList_MGF.cpp

xjasg · 2024-12-02T19:40:35Z

@chambm: Hi Matt, have you had a chance to look at the latest changes again? Should I change anything else? Thank you very much. Best regards.

chambm · 2025-01-15T21:34:04Z

Thanks @xjasg and @Andre99999999 !

xjasg · 2025-01-16T19:00:30Z

@chambm : I have to thank you for your kind support.

xjasg and others added 11 commits September 4, 2024 16:43

Feature Reader Bruker with extended precursor info

ba475f1

Collisional Cross Sectional Area added

c9f071e

mzXML and MGF Reader/Writer externded

5c7e77a

Bugfix Typo

e8e5c5e

Changes after review

112bf17

Bugfix merges

0b8ffc4

Test files changed

ee17fef

lowest and highest observed mz

8aeecd0

small cleanup

0f3b1a3

Merge branch 'master' into feature/bruker_precursor_2

8104588

Bugfix

3ab4cff

xjasg force-pushed the feature/bruker_precursor_2 branch from ac53357 to 3ab4cff Compare October 11, 2024 00:29

chambm requested changes Oct 11, 2024

View reviewed changes

Andre99999999 added 7 commits October 11, 2024 20:42

Fix Build exception

912c3c6

Bugfix unused header

56949cf

Revert changes after review

e7bf0d5

Remove unnecessary code

3e7fd96

Remove unused function

5f5a974

cleanup

ef4e5d4

Revert "lowest and highest observed mz"

e2e6ea2

This reverts commit 8aeecd0.

chambm reviewed Oct 15, 2024

View reviewed changes

Partially revert last changes ( add intensity population again )

bb1a8ee

xjasg force-pushed the feature/bruker_precursor_2 branch from 913253a to bb1a8ee Compare November 1, 2024 00:40

Andre99999999 added 5 commits November 1, 2024 01:48

Whitespace issues

853d207

merge latest version

701ac75

Whitespace issue

a80d333

MGF Serializer + Spectrum list changes (Revwiew findings)

f4213a0

Bugfix Build error

e487769

chambm self-requested a review November 6, 2024 19:55

chambm requested changes Nov 6, 2024

View reviewed changes

Extend MGF test with new parameters

73aca39

xjasg requested a review from chambm November 8, 2024 23:05

chambm reviewed Nov 12, 2024

View reviewed changes

pwiz/data/msdata/SpectrumList_MGF.cpp Outdated Show resolved Hide resolved

Cleanup

c18d414

xjasg requested a review from chambm November 12, 2024 19:41

Andre99999999 and others added 3 commits December 27, 2024 22:01

Bugfix remove ccs Parameter from title after mgf file was read

3cb5ae3

Merge branch 'master' into feature/bruker_precursor_2

924b415

Merge branch 'master' into feature/bruker_precursor_2

3587b11

chambm merged commit a2cc5cc into ProteoWizard:master Jan 15, 2025
15 checks passed

chambm deleted the feature/bruker_precursor_2 branch January 16, 2025 19:15

		@@ -228,14 +228,17 @@ struct PWIZ_API_DECL DiffConfig : public pwiz::data::BaseDiffConfig

		bool ignoreDataProcessing;

		bool ignoreSpectrumTitle;

Extension of bruker precursor information (2nd attempt) #3191

Extension of bruker precursor information (2nd attempt) #3191

Conversation

xjasg commented Oct 10, 2024

chambm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xjasg commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chambm left a comment

Choose a reason for hiding this comment

xjasg commented Nov 6, 2024

chambm commented Nov 6, 2024

xjasg commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xjasg commented Dec 2, 2024

chambm commented Jan 15, 2025

xjasg commented Jan 16, 2025