Skip to content
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

Msangel compatibility #543

Merged
merged 51 commits into from
Jan 27, 2025
Merged

Msangel compatibility #543

merged 51 commits into from
Jan 27, 2025

Conversation

mlocardpaulet
Copy link
Contributor

Parameter parsing of MSAngel json files for Mascot searches.
The parsing will be a bit different with other search engines (like X!Tandem - see file test/params/MSAngel_Xtandem-export-param.json). I am working on it!

@mlocardpaulet mlocardpaulet requested a review from enryH January 21, 2025 13:42
@mlocardpaulet
Copy link
Contributor Author

also, I still have to make the python file for testing parameter parsing.

@@ -89,6 +89,6 @@ class ProteoBenchParameters:
max_precursor_charge: Optional[int] = None
scan_window: Optional[int] = None # DIA-specific
quantification_method: Optional[str] = None
second_pass: Optional[bool] = None # DIANN specific
second_pass: Optional[bool] = None # used in both DDA and DIA: same thing?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is new?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, yes. It is not "DIANN specific", so I changed the comment.
I am sorry the new comment is not super clear. I just was not sure that it is exactly the same thing for dia and dda. But within each, I think that "second pass" is understandable. So it wont be an issue when we have module-specific parameters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is DIA specific though isnt it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, we also say "two-pass search" (in MQ, X!Tandem, maybe others...) when we do two successive searches with different tolerances.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in DDA

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and it is a good parameter to have (I thought we did) in DDA

@mlocardpaulet
Copy link
Contributor Author

... And I just realised! I forgot to add the "hidden modifications of X!Tandem" "quickPyrolidone" : true,!
I have to do it, but I am not entirely sure how yet.

Copy link
Member

@enryH enryH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code wise it looks good. But for now not too many parameters could be parsed looking at the extracted csv files.

@mlocardpaulet
Copy link
Contributor Author

Code wise it looks good. But for now not too many parameters could be parsed looking at the extracted csv files.

I fully agree. I think that I can maybe add a few with David's help. But I am not sure how much we'll cover actually.

@@ -93,7 +93,10 @@ def test_flatten_of_dicts(dict_in, list_expected):
assert actual == list_expected


parameters = [(fname, fname.with_suffix(".csv")) for fname in mqpar_fnames]
# TODO the test is broken, partly due to the expected files being incorrect
# TODO skip for now, fix in future
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can make an issue and assign me... It was hacky from the start

assert actual.equals(expected)


parameters = [(fname, (fname.parent / (fname.stem + "_sel.json"))) for fname in mqpar_fnames]
# TODO the test is broken, partly due to the expected files being incorrect
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null changed to NaN - could be the issue (None vs NaN). but could be good to normalize that.

@RobbinBouwmeester
Copy link
Contributor

@enryH thank you for already looking, but I need to fix a lot here still ;)

@RobbinBouwmeester
Copy link
Contributor

image

Seems to be working with manual input!

@RobbinBouwmeester
Copy link
Contributor

Ok, everything seems to be working! Only thing that is left to do is indicate manually changed params in the PR.

@RobbinBouwmeester
Copy link
Contributor

Ok, it works! I am going to merge with main, we can do a little bit of testing and release a new version.

image

@RobbinBouwmeester RobbinBouwmeester merged commit 24e7ff1 into main Jan 27, 2025
8 checks passed
@RobbinBouwmeester RobbinBouwmeester deleted the msangel-compatibility branch January 27, 2025 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants