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

GetClosestOneBasedSpectrumNumber using Binary Search instead of for loop #716

Closed

Conversation

elaboy
Copy link
Contributor

@elaboy elaboy commented Jul 5, 2023

The new method uses the Binary Search algorithm to search the closes spectrum of given retention time instead of a for loop.

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #716 (13d3cd0) into master (26e4eef) will increase coverage by 0.00%.
Report is 1 commits behind head on master.
The diff coverage is 91.66%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #716   +/-   ##
=======================================
  Coverage   75.95%   75.96%           
=======================================
  Files         170      170           
  Lines       28801    28823   +22     
=======================================
+ Hits        21876    21895   +19     
- Misses       6925     6928    +3     
Files Changed Coverage Δ
mzLib/MassSpectrometry/MsDataScan.cs 86.87% <ø> (ø)
mzLib/MassSpectrometry/MsDataFile.cs 95.23% <86.36%> (-2.64%) ⬇️
mzLib/Readers/Mgf/Mgf.cs 94.81% <100.00%> (+0.31%) ⬆️

@elaboy elaboy added Performance Maintenance The user isn't impacted by it, it's purely behind the scenes ready for review labels Jul 5, 2023
Copy link
Contributor

@Alexander-Sol Alexander-Sol left a comment

Choose a reason for hiding this comment

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

I left a couple of comments, but I see one major issue: A dictionary isn't an ordered collection, so accessing dictionary members by an index will give wacky results sometimes

@elaboy
Copy link
Contributor Author

elaboy commented Jul 6, 2023

I left a couple of comments, but I see one major issue: A dictionary isn't an ordered collection, so accessing dictionary members by an index will give wacky results sometimes

You are right. I changed it to InmmutableSortedDictionary and it is inside the method now.

nbollis
nbollis previously approved these changes Jul 26, 2023
@elaboy elaboy dismissed Alexander-Sol’s stale review August 1, 2023 18:02

This feature was eliminated from the code

mzLib/MassSpectrometry/MsDataFile.cs Outdated Show resolved Hide resolved
mzLib/MassSpectrometry/MsDataFile.cs Outdated Show resolved Hide resolved
mzLib/MassSpectrometry/MsDataFile.cs Show resolved Hide resolved
mzLib/MassSpectrometry/MsDataFile.cs Outdated Show resolved Hide resolved
mzLib/Test/FileReadingTests/TestMsDataFile.cs Outdated Show resolved Hide resolved
mzLib/Test/FileReadingTests/TestMsDataFile.cs Outdated Show resolved Hide resolved
mzLib/Test/FileReadingTests/TestMsDataFile.cs Outdated Show resolved Hide resolved
mzLib/Test/FileReadingTests/TestMsDataFile.cs Outdated Show resolved Hide resolved
kyp4
kyp4 previously approved these changes Aug 22, 2023
Copy link
Contributor

@kyp4 kyp4 left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

@elaboy elaboy requested review from trishorts and kyp4 August 30, 2023 15:45
@elaboy elaboy closed this Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance The user isn't impacted by it, it's purely behind the scenes Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants