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
Much needed TLC for AtomContainer #321
Conversation
…dead code (commented out). No other changes.
… of range else we end up with 'null' atom gaps, this also means we don't need to worry about the atom count which can be incorrect with the current behaviour.
…eo elements so we can replace atoms easier. No tests regressions.
…mistry when an atom is set.
…mContainer - we'll move the Sgroups later.
…ly want to leave the molecule in a consistent state. However the rarer more exotic method is still useful but we name it as such. "Make it easy to do the commonly correct things, make it possible to do the exotic" - https://youtu.be/wbp-3BJWsU8?t=8m6s
…Exception but that breaks some code in cdk-reaction.
…tation to reflect an exception should now be thrown on out of bounds.
…iour for any capacitity, previously we may get a null or an ArrayIndexOutOfBounds.
… order 4 and handle the corner cases. It's not clear whether the implicit hydrogens should be included but it makes sense this method is invariant depending on whether hydrogens are implicit/explicit. Also throw an exception if the atom is not in the container - whilst the silent failing state is nice sometimes it usually means something else has gone wrong. NoSuchAtomException fits better but is unfortunately checked so can't be used ATM.
…t checks on the -1 charge.
…uld +1 and -1... this matches the expected value and matches the non charge seperate (pyridine).
…n if the atom is not in the molecule, regressions expected!
…eck connectivity in non-exclusion containers.
… we can no longer to the quick circuit rank calculation. Also ensure molecule is rolled back correctly.
@egonw Sorry for large patch, would it be possible to get this merged in this weekend? Wanted to do some more CDK work today but am waiting on this. Thanks. |
"Also deprecate getConnectedAtomCount which does nothing different from getConnectedBondCount" Of course, except for the multi atom bonds :) |
but it didn't actually do that :-). |
That doesn't surprise me... over time original ideas and implementations diverged... |
Clean up implementations and tighten up IAtomContainer API. A big one but should hopefully read okay. Here's a summary of each step. The tightening of the API causes some regressions which highlighted some simple mistakes (e.g. wrong variable being passed in).
Array growth
The growing of the backing arrays has been tweaked to follow the same scheme
as Java's ArrayList. We now grow by x1.5 rather than the fixed +10. Using
ArrayList as the backing array is not possible at the moment as there are
current modifications deep in cdk-reaction, something to look at in future
though.
setAtom
Don't allow atoms to be set in any position (atomCount was not incremented)
also keep the container in a consistent state with regards to bonds etc. This
removes much of the need for the replaceAtomWithAtom utility function.
setAtoms/setBonds
Copy the arrays for safety/encapsulation. Could also wipe bonds on setAtoms()
but we'll leave that for now.
removeAtom/removeAtomAndConnectedElectronContainers
Add a new method
removeAtomOnly
that does whatremoveAtom
used to do,removeAtom
now also removes bonds and we deprecatedremoveAtomAndConnectedElectronContainers
. In existing code we're good becauseremoveAtom(atom)
followed byremoveBond(bond)
is extra work but safe.getAtom/getBond/getSingleElectron
Bounds check the index. Currently we have situations like this:
getMinBondOrder/getMaxBondOrder
Correct implementations and throw error for atoms not in the container.
getBondOrderSum
also needs fixing but I think it'll be better to deprecate it.
getConnected...
We now throw an error if an atom does not belonging to the container is passed in.
Also deprecate getConnectedAtomCount which does nothing different from getConnectedBondCount.
I intend to replace the usage of NoSuchElementException with NoSuchAtomException but
will do that once that runtime patch is applied.