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

(wip) Additional window groups splitting strategies. #20

Closed

Conversation

jspaezp
Copy link
Contributor

@jspaezp jspaezp commented Jul 12, 2024

This is a pretty rough implementation of alternative ways to split DIA frames into raw spectra.

If this is within the scope of what you want to support I would be happy to finish it as a full feature (+ add testing, + add documentation/comments, + remove debug prints)

LMK what you think! @sander-willems-bruker

@@ -50,6 +36,15 @@ impl DIARawSpectrumReader {
impl RawSpectrumReaderTrait for DIARawSpectrumReader {
fn get(&self, index: usize) -> RawSpectrum {
let quad_settings = &self.expanded_quadrupole_settings[index];
if index < 10 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is here just for debugging ...

// Read the 'NUM_SUB_SUB_SPLITS' from env variables ... default to 1
// (for now)

let splits = match std::env::var("NUM_SUB_SUB_SPLITS") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Obviously this would be passed as an argument and not read from the env. variables. IMO

@jspaezp jspaezp marked this pull request as draft July 12, 2024 20:54
@jspaezp
Copy link
Contributor Author

jspaezp commented Jul 12, 2024

This would also be a good time to expose the smoothing-centroiding parameters to the public API ... which would be something that gets passed to timsrust::io::readers::SpectrumReader::new ... which could be something like ...

struct SpectrumReaderParameters {
    spectrum_processing_params: <... smoothing and centroiding stuff ...>,
    frame_splitting_params: Option<FrameSplittingStrategy>
}

@jspaezp jspaezp changed the title (wip) added span-run and nsplit strategies (wip) Additional window groups splitting strategies. Jul 12, 2024
@jspaezp
Copy link
Contributor Author

jspaezp commented Jul 13, 2024

An additional thing that could go into the strategy would be whether it should be done at the quad+window level or only at the window level (which would enable diagonal pasef methods to be split as well).

@sander-willems-bruker sander-willems-bruker changed the base branch from main to develop July 15, 2024 07:24
@sander-willems-bruker
Copy link
Collaborator

Thanks for the PR, looks very nice! Tested it out locally and can confirm your findings with regards to speed and identification performance. If you are already working on making it a clean PR, I'll let you finish that up. If you haven't started yet though, I can also take it from here and work on it locallu before merging into dev.

@sander-willems-bruker
Copy link
Collaborator

This would also be a good time to expose the smoothing-centroiding parameters to the public API ... which would be something that gets passed to timsrust::io::readers::SpectrumReader::new ... which could be something like ...

struct SpectrumReaderParameters {
    spectrum_processing_params: <... smoothing and centroiding stuff ...>,
    frame_splitting_params: Option<FrameSplittingStrategy>
}

This indeed is definately something that needs to happen indeed. For now, leave it to me, I have some local developments in sage that will also utilize this. DiagonalPASEF is still onb my todo list, don't have a clear idea how I would integrate that as clean as possible although your implementation serves as a very decent start for brainstorming

@jspaezp
Copy link
Contributor Author

jspaezp commented Jul 15, 2024

Right on! I will propagate some of the changes and implement the strategy pattern for the config! (I first wanted to know that this was something you would be interested in before going ahead and "back-propagating" the configuration)

@jspaezp
Copy link
Contributor Author

jspaezp commented Jul 17, 2024

@sander-willems-bruker Just FYI your last push to develop introduced A LOT of merge conflicts. I would have appreciated a heads up.

reference: https://github.com/MannLabs/timsrust/compare/cc83557b5dd6ed567abd2d72780726edd2c55b1a..21efdf9e5279a200195c4bdb61fdd7738e219b66

@jspaezp jspaezp closed this Jul 17, 2024
@jspaezp jspaezp force-pushed the feature/alternative_dia_splitting branch from f5fb8d9 to cc83557 Compare July 17, 2024 07:03
@sander-willems-bruker
Copy link
Collaborator

Yes, my apologies. I should have made a separate branch before merging to develop. Will try to make that my new routine for future work. We'll continue on the other PR;)

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