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.

Sign up for GitHub

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

Patch/refactor peak picking #27

Merged
merged 11 commits into from
Nov 14, 2023
Merged

Patch/refactor peak picking #27

merged 11 commits into from
Nov 14, 2023

Conversation

jcharkow
Copy link
Collaborator

@jcharkow jcharkow commented Nov 9, 2023

Note: This PR is an extension (dependent on) #26

Here I implement both pyopenms TransitionGroupPicker and refactoring of Justin's python implementation of TransitionGroupPicker

Please see the other PR mentioned above first

@jcharkow jcharkow requested a review from singjc November 9, 2023 15:22
@jcharkow
Copy link
Collaborator Author

jcharkow commented Nov 9, 2023

Note: Test still have to be added will be doing that soon

@jcharkow
Add tests for MRMPeakPicker.py
1d06c0a 
Also debug existing code while implementing tests
@jcharkow
[REFACTOR][TEST]
23825f8 
refactor pyMRMTransitionGroupPicker and add tests for it
@jcharkow
[TEST] add chromatogram test
4edd0f3
@jcharkow
update transitionFeature
fab6eab 
add method to convert list of transitionFeatures to a dataframe
@jcharkow
Copy link
Collaborator Author

All the tests should be resolved now. I have not had a chance to test peak picking works as expected outside of test cases though

@jcharkow
Copy link
Collaborator Author

@singjc Please make sure the logic of pyMRMPeakPicker still makes sense. I am using a pd dataframe to increase readability.

@jcharkow jcharkow requested a review from singjc November 13, 2023 21:38
@singjc
Copy link
Collaborator

singjc commented Nov 14, 2023

@singjc Please make sure the logic of pyMRMPeakPicker still makes sense. I am using a pd dataframe to increase readability.

Looks good to me 👍
I think we just need to fix the recent conflict in the Chromatgoram struct since it contains the empty method in the main masterRefactor branch, I think that's conflicting

@jcharkow
Copy link
Collaborator Author

Fixed merge conflict so will merge now!

@jcharkow jcharkow merged commit 1841ee5 into patch/masterRefactor Nov 14, 2023
@singjc
Copy link
Collaborator

singjc commented Nov 14, 2023

Great! Could we delete these branches once they're merged if they're not going to be used/expanded?

@jcharkow jcharkow deleted the patch/refactor_peakPicking branch November 14, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@singjc singjc Awaiting requested review from singjc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jcharkow @singjc