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

Open
sneumann opened this issue Apr 30, 2020 · 8 comments
Open

Record format issue #235

sneumann opened this issue Apr 30, 2020 · 8 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 ...
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
3 participants
@sneumann @tsufz @schymane
You can’t perform that action at this time.