Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

only delete non-multibond H's; fixes JCP issue 8 #28

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

rwst commented Aug 8, 2012

this the fix premeditated on the mailing list allowing molecules H3C-H-CH3 to be cleaned without hiccup

the link to the JCP issue is JChemPaint/jchempaint#8

@johnmay johnmay and 1 other commented on an outdated diff Aug 8, 2012

...openscience/cdk/layout/StructureDiagramGenerator.java
//IAtom[] atoms = shallowCopy.getAtoms();
- for (int i = 0; i < shallowCopy.getAtomCount(); i++) {
- IAtom curAtom = shallowCopy.getAtom(i);
- if (curAtom.getSymbol().equals("H")) {
- shallowCopy.removeAtomAndConnectedElectronContainers(curAtom);
- curAtom.setPoint2d(null);
+ for (IAtom curAtom : shallowCopy.atoms()) {
+ if (curAtom.getSymbol().equals("H")) {
+ int bondsFromCurAtom=0;
+ for (IBond bond : shallowCopy.bonds())
+ if(bond.contains (curAtom))
+ ++bondsFromCurAtom;
+ if (bondsFromCurAtom < 2) {
@rwst

rwst Aug 8, 2012

Contributor

Done. Thanks for the hint.

Owner

egonw commented Aug 23, 2012

John, did you make signed of copies of these two patches?

Owner

egonw commented Aug 23, 2012

But since this is a bug fix for cdk-1.4.x, it also needs a unit test... I suggest a test that checks if the fully-connected starting structure, is still fully connected after the SDG call, pretty much the problem outlined in the JCP report...

Owner

johnmay commented Aug 23, 2012

No not yet. As you say a unit test would be good.

Did you change the commit back from using the manipulator ralf?

Thanks,
J

Owner

egonw commented Aug 23, 2012

John, I think he made an additional commit, see the pull request, which now has two commits...

This is the update commit: https://github.com/rwst/cdk/commit/869829aacaab0e3cc040641b5479dd03633e12d8

Owner

johnmay commented Aug 23, 2012

Ah, I remember now

Owner

egonw commented Sep 17, 2012

I have added a unit test:

https://github.com/egonw/cdk/commits/422-14x-middleHUnitTest

I also signed of Ralf's patches which fix the fail of this unit test.

@rwst rwst closed this Oct 6, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment