search

MassBank / MassBank-web

The web server application and directly connected components for a MassBank web server
13 stars 22 forks source link

Record format issue #235

Closed sneumann closed 4 years ago

sneumann commented 4 years ago

Hi, we got a report about an inconsistency 

an error in the MassBank Record Format 2.5.
[Error] 2.5.1 Subtag: PRECURSOR_MZ
[Correct] 2.5.1 Subtag: PRECURSOR_M/Z

"/" is missing in the current format.

The PRECURSOR_M/Z was indeed present in the MassBankRecordFormat_en.pdf files up to at least 2017, and also still in https://github.com/MassBank/MassBank-web/blob/9c1ff657782ced8cc830bd3da0a5a39a018f5821/Documentation/MassBankRecordFormat.md#251-subtag-precursor_mz

Yours, Steffen

schymane commented 4 years ago

I guess we decided to avoid the / if possible, but didn't update validation properly? What is the current format in MassBank-data records?

sneumann commented 4 years ago

Hunting the change down, it was still M/Z in 2018: https://github.com/MassBank/MassBank-web/blob/b0236c387938c0aa582dcc20c429de63e846f688/Documentation/MassBankRecordFormat.md#251-subtag-precursor_mz

sneumann commented 4 years ago

The change was part of this commit https://github.com/MassBank/MassBank-web/commit/6208e9a01560c28223cf981d45fd20216bd33fcb

The PRECURSOR_M/Z was mentioned in this issue https://github.com/MassBank/MassBank-web/issues/96 before, and deemed valid.

Can @tsufz and @meier-rene comment whether we had a discussion preceding the change to PRECURSOR_MZ ? Yours, Steffen

tsufz commented 4 years ago

Yes, there was a short discussion in https://github.com/MassBank/MassBank-web/issues/200#issuecomment-546857625. @meowcat asked to avoid slashes w/o further comments. Thus, I removed the slashes.

tsufz commented 4 years ago

In https://github.com/MassBank/MassBank-web/issues/200#issuecomment-546866390, I asked for comments on the proposal of the updated record format. I have no problems with the slashes, but the all tags should be the same.

tsufz commented 4 years ago

There are related open issues to update the records to be compliant with the new record format if appropriate:

Hence, no record is changed so far. Happy to discuss all issues and the updated record format.

Best, Tobias

sneumann commented 4 years ago

I would expect a lot of code out there in the world uses the PRECURSOR_M/Z of the existing records, and I would be strongly against mass-changing the records. Instead I would like to see our documentation adapted/reverted to the old state. Yours, Steffen

schymane commented 4 years ago

Agree with @sneumann ...

tsufz commented 4 years ago

I have no problems to adapt the old stage. However, could you check the other suggestions, please. I am also no friend of mass changes of records. But in my opinion it is necessary have consistent tags. See https://github.com/MassBank/MassBank-data/issues/99 and https://github.com/MassBank/MassBank-data/issues/98.

Comment the new record format version and the suggested changes and then we can change / revert.

Txs.

meier-rene commented 4 years ago

I checked all records. We dont have any records with _MZ tags. All records have _M/Z. There are two tags: MASS_RANGE_M/Z and PRECURSOR_M/Z. I dont care about / in tag names and to prevent mass change of records without any benefit I will revert format spec to to _M/Z.

schymane commented 4 years ago

Sounds good to me @meier-rene Can you pls check MassBank? it's looking ill ...

meier-rene commented 4 years ago

Yes, we know, but dont know the reason.

schymane commented 4 years ago

OK, glad you know and are on to it ... good luck & let me know if you need assistance in any way ...

meier-rene commented 4 years ago

Done with 7447c2f502ad70af5122885971b31f7d049cdcd9.