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

Development #130

Merged
merged 7 commits into from
Jan 14, 2024
Merged

Development #130

merged 7 commits into from
Jan 14, 2024

Conversation

jalew188
Copy link
Collaborator

In spectral_library.base, calc_precursor_isotope will use calc_precursor_isotope_intensity. It will be the default option for all speclib generation if isotopes are included.

I also removed multiprocessing=True parameter as it can be controled by mp_process_num>1. @GeorgWa Please be aware this change to alphadia.

@jalew188 jalew188 requested a review from GeorgWa January 13, 2024 07:48
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Collaborator

@GeorgWa GeorgWa 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 for me!

"""
fragment.update_precursor_mz(self._precursor_df)

def update_precursor_mz(self):
def calc_and_clip_precursor_mz(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

really good, I like that the clipping is mentioned and won't surprise users

min_right_most_intensity = min_right_most_intensity,
)


def calc_precursor_isotope(self,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you like we can keep only one calc_precursor_isotope_intensity or calc_precursor_isotope so there's no redundancy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

calc_precursor_isotope is now calc_precursor_isotope_info. My original thought is that maybe it is useful in AlphaDIA, but I think your calc_*_intensity is better in readability and memory-saving.

@jalew188
Copy link
Collaborator Author

jalew188 commented Jan 14, 2024

@GeorgWa If this PR does not disturb some AlphaDIA functionalities, we can them merge it.

@jalew188 jalew188 merged commit 3237027 into main Jan 14, 2024
8 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