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

Added method to retrieve the raw spectrum from any USI #15

Merged
merged 12 commits into from
Nov 1, 2024

Conversation

douweschulte
Copy link
Contributor

This is a new PR with fixed history (squashed all my earlier commits into one). See #14 for the previous discussion.

@douweschulte
Copy link
Contributor Author

I fixed the CURIE problem by having the PROXI acession be either a CURIE or a string in that way all metadata comes over and all logic still works with the drawback of the structure being slightly more complex. With this all my tests are positive and I made all these tests also run in the github action. With that I am ready for merging, let me know if there are other things you want to have fixed before merging.

@douweschulte douweschulte marked this pull request as ready for review October 30, 2024 10:00
src/io/proxi.rs Show resolved Hide resolved
src/io/proxi.rs Outdated Show resolved Hide resolved
src/io/proxi.rs Outdated Show resolved Hide resolved
src/io/proxi.rs Outdated
/// aggregate the results from all known backends and return the first successful spectrum.
///
/// This function is only available with the feature `proxi`.
pub fn get_spectrum_blocking(
Copy link
Owner

Choose a reason for hiding this comment

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

Stylistically I'm tempted to ask this be renamed to something more succinct but "proxi" is meaningless to many new users and I've no better ideas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fully agree, but also did not find a better name yet. Maybe 'retrieve'/'fetch'/'load' with or without the 'spectrum' part?

Copy link
Contributor Author

@douweschulte douweschulte Oct 31, 2024

Choose a reason for hiding this comment

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

pytoemics calls the function 'proxi()' so we might get away with 'proxi_blocking()' and 'proxi_async' especially if we link to that in the main documentation for USI.

Copy link
Owner

Choose a reason for hiding this comment

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

Since I wrote the pyteomics implementation, I was partial to that here too. PROXI was still being worked on in recent memory way back then too. The mzSpecLib project has taken all the attention away from PROXI since then, and it's on the "todo" list to switch the PROXI JSON format to mirror the mzSpecLib JSON format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reading a bit more into the PROXI standard I do agree that calling it .proxi is not the best name, if only for the fact that proxi allows many more types of calls to the server. Keeping it on get_spectrum seems fine to me, that is also roughly the name of the proxi 'function' that is called in the function. Although the 'get' part could be swapped out to indicate the network request a bit more, something like 'download_spectrum' might be more clear to anyone reading that it does network requests.

src/io/proxi.rs Outdated Show resolved Hide resolved
@douweschulte
Copy link
Contributor Author

While using this implementation in the annotator I found a small bug with parsing the signal continuity. Otherwise it works like a charm. I renamed the function to download_spectrum_blocking/async which I think clearly states the point of the function: "you will get the spectrum, downloaded over the internet". The last factor, downloading, was not clear enough with the previous name and I think making it obvious in this way would flag more clearly to users that the internet is involved.

With that I am ready for merging, do you have any questions/comments left?

@mobiusklein mobiusklein merged commit 7f348bc into mobiusklein:main Nov 1, 2024
2 checks passed
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.

2 participants