Skip to content

/ MassBank-web

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Record format issue #235

Closed
sneumann opened this issue Apr 30, 2020 · 14 comments
Closed

Record format issue #235

sneumann opened this issue Apr 30, 2020 · 14 comments

Comments

@sneumann
Copy link
Member

@sneumann sneumann commented Apr 30, 2020 •
edited

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
Copy link
Member

@schymane schymane commented Apr 30, 2020

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
Copy link
Member Author

@sneumann sneumann commented Apr 30, 2020

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
Copy link
Member Author

@sneumann sneumann commented Apr 30, 2020

The change was part of this commit 6208e9a

The PRECURSOR_M/Z was mentioned in this issue #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
Copy link
Member

@tsufz tsufz commented Apr 30, 2020 •
edited

Yes, there was a short discussion in #200 (comment). @meowcat asked to avoid slashes w/o further comments. Thus, I removed the slashes.
@tsufz
Copy link
Member

@tsufz tsufz commented Apr 30, 2020 •
edited

In #200 (comment), 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
Copy link
Member

@tsufz tsufz commented Apr 30, 2020

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
Copy link
Member Author

@sneumann sneumann commented May 1, 2020

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
Copy link
Member

@schymane schymane commented May 1, 2020

Agree with @sneumann ...
@tsufz
Copy link
Member

@tsufz tsufz commented May 3, 2020 •
edited

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 MassBank/MassBank-data#99 and MassBank/MassBank-data#98.

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

Txs.
@meier-rene
Copy link
Contributor

@meier-rene meier-rene commented May 6, 2020

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
Copy link
Member

@schymane schymane commented May 6, 2020

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

@meier-rene meier-rene commented May 6, 2020

Yes, we know, but dont know the reason.
@schymane
Copy link
Member

@schymane schymane commented May 6, 2020

OK, glad you know and are on to it ... good luck & let me know if you need assistance in any way ...
meier-rene pushed a commit that referenced this issue May 6, 2020
René Meier
Revert to _M/Z in tags as discussed in #235
Loading status checks…
7447c2f
@meier-rene
Copy link
Contributor

@meier-rene meier-rene commented May 6, 2020

Done with 7447c2f.
@meier-rene meier-rene closed this May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
@sneumann @tsufz @schymane @meier-rene
You can’t perform that action at this time.