-
Notifications
You must be signed in to change notification settings - Fork 870
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
Valence clean-up #7397
Open
ptosco
wants to merge
2
commits into
rdkit:master
Choose a base branch
from
ptosco:valence-cleanup
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Valence clean-up #7397
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…mbership Al(IIIb), Si(IVb), P(Vb), As(Vb), Sb(Vb), Bi(Vb) - address elements with d orbitals (P, As, Sb, Si) which can accommodate bonds supported by lone pairs from halogens and chalcogens with requiring adding extra valences to periodic table - add test to make sure that PX6-, AlX63-, AsX6-, SbX6-, BiF6- do not raise an AtomValenceException while PX6, AlX6, AsX6, SbX6, BiX6 and peracids of the same elements which cannot exist raise an AtomValenceException - correct SmilesTests.java and rdkit/Chem/UnitTestSmiles.py where CCC[Al+3] should rather read CCC[Al+2] - comment out incorrect entries in rdkit/Chem/test_data/NCI_aromat_regress.txt and rdkit/Chem/test_data/NCI_5K_TPSA.csv - add -1 valence to alkali elements which were missing it (Rb, Cs, Fr)
greglandrum
added a commit
to greglandrum/rdkit
that referenced
this pull request
Jun 5, 2024
greglandrum
added a commit
that referenced
this pull request
Jun 25, 2024
* change valence model to use isolobal analogy Remove support for five-coordinate C+ and, by analogy, five-coordinate N+2 Removes support for charge states that take atoms past the end of the periodic table i.e. [Lv-4] is no longer supported * update the tests for that * remove valence state of 6 for Al * fix representation of phosphate in the mol2 parser this is a correction of what was done during #5973 * cleanup the exceptions for P, S, As, and Se * drop valence states: Si 6, P 7, As 7 * a couple of additional changes from #7397 * update java tests * fix an inconsistency: Rb now supports valence -1 * documentation * - replace operator[] with at() for bounds check - extract some code into a function to avoid duplication - use TAB as separator throughout in the periodic table data for consistency * removing the .at() usage We know that these vectors aren't empty, so there's no need for the bounds check. --------- Co-authored-by: ptosco <[email protected]>
e-kwsm
pushed a commit
to e-kwsm/rdkit
that referenced
this pull request
Jun 30, 2024
* change valence model to use isolobal analogy Remove support for five-coordinate C+ and, by analogy, five-coordinate N+2 Removes support for charge states that take atoms past the end of the periodic table i.e. [Lv-4] is no longer supported * update the tests for that * remove valence state of 6 for Al * fix representation of phosphate in the mol2 parser this is a correction of what was done during rdkit#5973 * cleanup the exceptions for P, S, As, and Se * drop valence states: Si 6, P 7, As 7 * a couple of additional changes from rdkit#7397 * update java tests * fix an inconsistency: Rb now supports valence -1 * documentation * - replace operator[] with at() for bounds check - extract some code into a function to avoid duplication - use TAB as separator throughout in the periodic table data for consistency * removing the .at() usage We know that these vectors aren't empty, so there's no need for the bounds check. --------- Co-authored-by: ptosco <[email protected]>
e-kwsm
pushed a commit
to e-kwsm/rdkit
that referenced
this pull request
Jul 8, 2024
* change valence model to use isolobal analogy Remove support for five-coordinate C+ and, by analogy, five-coordinate N+2 Removes support for charge states that take atoms past the end of the periodic table i.e. [Lv-4] is no longer supported * update the tests for that * remove valence state of 6 for Al * fix representation of phosphate in the mol2 parser this is a correction of what was done during rdkit#5973 * cleanup the exceptions for P, S, As, and Se * drop valence states: Si 6, P 7, As 7 * a couple of additional changes from rdkit#7397 * update java tests * fix an inconsistency: Rb now supports valence -1 * documentation * - replace operator[] with at() for bounds check - extract some code into a function to avoid duplication - use TAB as separator throughout in the periodic table data for consistency * removing the .at() usage We know that these vectors aren't empty, so there's no need for the bounds check. --------- Co-authored-by: ptosco <[email protected]>
yishutu
pushed a commit
to yishutu/rdkit
that referenced
this pull request
Aug 15, 2024
* change valence model to use isolobal analogy Remove support for five-coordinate C+ and, by analogy, five-coordinate N+2 Removes support for charge states that take atoms past the end of the periodic table i.e. [Lv-4] is no longer supported * update the tests for that * remove valence state of 6 for Al * fix representation of phosphate in the mol2 parser this is a correction of what was done during rdkit#5973 * cleanup the exceptions for P, S, As, and Se * drop valence states: Si 6, P 7, As 7 * a couple of additional changes from rdkit#7397 * update java tests * fix an inconsistency: Rb now supports valence -1 * documentation * - replace operator[] with at() for bounds check - extract some code into a function to avoid duplication - use TAB as separator throughout in the periodic table data for consistency * removing the .at() usage We know that these vectors aren't empty, so there's no need for the bounds check. --------- Co-authored-by: ptosco <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR cleans up valences in
atomic_data.cpp
. In addiition to a few missing-1
valences for some alkali metals (Rb, Cs, Fr), which I believe should be added in analogy to Li, Na and K, there were a number of higher valences for Al (6), Si (6), P, As, Sb, Bi (7) which I believe are incorrect. Al has only valence 3 (not 6), Si has only valence 4 (not 6), P, As, Sb, Bi can have 3 and 5 (not 7).The incorrect valences lead to non-existing compounds such as AlX6, PF6, AsF6, SbF6, BiF6 to be accepted.
The fact that these elements may form negatively charged complexes which higher valence thanks to their d orbitals can be more appropriately accounted for by modifying the code in
Atom.cpp
. With these modifications, all existing unit tests pass, while preventing species such as AlX6, PF6, AsF6, SbF6, BiF6 to be accepted.Two Java tests which were failing with the new code in this PR upon inspection turned out to refer to species which are actually incorrect and should be rejected. I have therefore amended such tests.
Incorrect species were:
while it should have rather been Al2(C2O42-)3
while it should have been
while it should have been