-
-
Notifications
You must be signed in to change notification settings - Fork 132
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
Remove Hydrogen isotopes in deprotonating #337
Conversation
Hydrogen isotopes should also be removed. Also, the documentation was old/wrong and now it is updated.
What kinds of tests should I add? |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Thanks @Ruibin-Liu ! I think the tests you've added seem sufficient to me; anything you're unhappy with? |
I think it's good to go, but I am not sure why some checks failed. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Looks like there was some dependency drift. If you sync up to |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Hydrogen isotopes should also be removed. Also, the documentation was old/wrong and now it is updated.
Reference Issues/PRs
NA
What does this implement/fix? Explain your changes
Removing
H
atoms byelement_symbol == "H"
doesn't remove isotopesD
orT
.Simply adding
D
andT
to the list should work.What testing did you do to verify the changes in this PR?
NA
Pull Request Checklist
./CHANGELOG.md
file (if applicable)./graphein/tests/*
directories (if applicable)./notebooks/
(if applicable)python -m py.test tests/
and make sure that all unit tests pass (for small modifications, it might be sufficient to only run the specific test file, e.g.,python -m py.test tests/protein/test_graphs.py
)black .
andisort .