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

PerceiveStereo option for Bayesian class #397

Merged
merged 4 commits into from Dec 12, 2017

Conversation

k-ujihara
Copy link
Contributor

A flag whether to perceive stereo or not is added to Bayesian class.
Rolling back CircularFingerprinter.rubricTetrahedral(int) method is also included.

@johnmay
Copy link
Member

johnmay commented Dec 10, 2017

What's the rollback for?

@k-ujihara
Copy link
Contributor Author

A calculated relative x-position of implicit H of 'atom' should be stored in 'xp[3]' at the following of "// build an implicit H if necessary" line. 'x0' is just the absolute x-position of 'atom' here.
The previous code looks good.

@johnmay
Copy link
Member

johnmay commented Dec 10, 2017

See #377, you get the wrong answer if you use that method. The better way is to use the XYZ of chiral atom.

@k-ujihara
Copy link
Contributor Author

I understood using XYZ is better. Thanks.
But, I believe

        xp[3] = x0;
        yp[3] = y0;
        zp[3] = z0;

is corrected to

        xp[3] = 0;
        yp[3] = 0;
        zp[3] = 0;

because (xp, yp, zp) is relative position in this context .
In fact, it fails the following test.

@Test
public void testNonZZeroPlaner() throws Exception {
    IAtomContainer mol = new AtomContainer();
    Atom[] atoms = new Atom[] {
        new Atom("C"),
        new Atom("F"),
        new Atom("N"),
        new Atom("O"),
    };
    atoms[0].setPoint3d(new Point3d(0, 0, -10));
    atoms[1].setPoint3d(new Point3d(0, 1, -10));
    atoms[2].setPoint3d(new Point3d(-1, -1, -10));
    atoms[3].setPoint3d(new Point3d(1, -1, -10));
    mol.setAtoms(atoms);
    mol.addBond(0, 1, IBond.Order.SINGLE);
    mol.addBond(0, 2, IBond.Order.SINGLE);
    mol.addBond(0, 3, IBond.Order.SINGLE);
    mol.getBond(0).setStereo(IBond.Stereo.UP);
    
    CircularFingerprinter circ = new CircularFingerprinter(CircularFingerprinter.CLASS_ECFP6);
    circ.setPerceiveStereo(true);
    IBitFingerprint fp0 = circ.getBitFingerprint(mol);
    
    for (int i = 0; i < atoms.length; i++) {
        Point3d p = atoms[i].getPoint3d();
        atoms[i].setPoint3d(new Point3d(p.getX(), p.getY(), p.getZ() + 20));
    }

    IBitFingerprint fp1 = circ.getBitFingerprint(mol);
    
    Assert.assertThat(fp0, is(fp1));
}

@johnmay
Copy link
Member

johnmay commented Dec 11, 2017

Yep you're right, could you make the patch for:

        xp[3] = 0;
        yp[3] = 0;
        zp[3] = 0;

and I'll pull that in.

@johnmay
Copy link
Member

johnmay commented Dec 12, 2017

Perfect, thanks!

@johnmay johnmay merged commit 528f9d7 into cdk:master Dec 12, 2017
@k-ujihara k-ujihara deleted the patch/Bayesian branch December 29, 2017 01:00
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