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

Much needed TLC for AtomContainer #321

Merged
merged 31 commits into from May 14, 2017
Merged

Much needed TLC for AtomContainer #321

merged 31 commits into from May 14, 2017

Conversation

johnmay
Copy link
Member

@johnmay johnmay commented May 7, 2017

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 what removeAtom used to do,
removeAtom now also removes bonds and we deprecated
removeAtomAndConnectedElectronContainers. In existing code we're good because
removeAtom(atom) followed by removeBond(bond) is extra work but safe.

getAtom/getBond/getSingleElectron

Bounds check the index. Currently we have situations like this:

IAtomContainer atom = new AtomContainer(10, 0, 0, 0);
atom.getAtom(0); // null
atom.getAtom(11); // ArrayIndexOutOfBounds

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.

johnmay added 30 commits May 7, 2017 10:28
…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.
…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.
…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.
@johnmay
Copy link
Member Author

johnmay commented May 13, 2017

@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.

@egonw egonw self-assigned this May 13, 2017
@egonw
Copy link
Member

egonw commented May 13, 2017

"Also deprecate getConnectedAtomCount which does nothing different from getConnectedBondCount"

Of course, except for the multi atom bonds :)

@egonw egonw merged commit f2274f2 into master May 14, 2017
@johnmay
Copy link
Member Author

johnmay commented May 14, 2017

but it didn't actually do that :-).

49887fc#diff-dbe5d4fee4e9a855d3f96521449265c5R674

@egonw
Copy link
Member

egonw commented May 14, 2017

That doesn't surprise me... over time original ideas and implementations diverged...

@johnmay johnmay deleted the cleanup/AtomContainer branch June 17, 2017 10:11
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.

None yet

2 participants