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

Add test of G-RxMC (ideal). #33

Merged
merged 6 commits into from
Apr 16, 2024
Merged

Add test of G-RxMC (ideal). #33

merged 6 commits into from
Apr 16, 2024

Conversation

davidbbeyer
Copy link
Contributor

Solves #32

@pm-blanco I have added a test that checks if the result of the G-RxMC implementation (both standard and unified) is consistent with the Henderson-Hasselbalch + Don. If you agree with the overall conception, I would add the same for the cpH-ensemble before we merge it.

@pm-blanco
Copy link
Collaborator

@davidbbeyer thank you for your initiative! I agree with the overall concept, but I would change a bit the implementation, comments will follow

samples/peptide_mixture_grxmc_ideal.py Outdated Show resolved Hide resolved
samples/peptide_mixture_grxmc_ideal.py Outdated Show resolved Hide resolved
samples/peptide_mixture_grxmc_ideal.py Outdated Show resolved Hide resolved
samples/peptide_mixture_grxmc_ideal.py Outdated Show resolved Hide resolved
@davidbbeyer
Copy link
Contributor Author

@pm-blanco I have implemented your suggestions

@pm-blanco
Copy link
Collaborator

@davidbbeyer I think the current implementation looks very good, please do a similar test for the cpH and we can proceed to merge this PR.

@davidbbeyer davidbbeyer marked this pull request as ready for review April 15, 2024 13:45
@davidbbeyer
Copy link
Contributor Author

@pm-blanco I have added a test on cpH

@pm-blanco
Copy link
Collaborator

@davidbbeyer the CI now breaks at the peptide tests, but they run locally for me. Maybe try increasing a bit the number of samples for the peptide tests to avoid ramdom crashes of the CI?

Copy link
Collaborator

@pm-blanco pm-blanco left a comment

Choose a reason for hiding this comment

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

It looks good to me now, thank you @davidbbeyer

@pm-blanco pm-blanco merged commit 688bab6 into pyMBE-dev:main Apr 16, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants