#183 Timeout for fingerprints

Nina Jeliazkova

It turns out there are few cases, when fingerprints take very long time and lot of memory (examples below)
This applies at least for FingerPrinter and ExtendedFingerprinter. Some of the other fingerprints also fail, because of AllRingsFinder timeout.
In practice with all the default settings it is hard to generate any kind of fingerprints for the example structures.

One easy workaround is to be able to set a timeout for fingerprints, similar to AllRingsFinder.
Yet another feature request would be to allow external AllRingsFinder for fingerprints, which depend on it. Some use an own instance in the constructor.

public void parseBad() throws Exception {
int n = 3; //don't run with n > 6, unless on a supercomputer
String[] problemStructures = {

    for (String inchi : problemStructures) {

        InChIGeneratorFactory f = InChIGeneratorFactory.getInstance();
        InChIToStructure c =f.getInChIToStructure(inchi, SilentChemObjectBuilder.getInstance());
        Assert.assertEquals(INCHI_RET.OKAY, c.getReturnStatus());
        IAtomContainer mol = c.getAtomContainer();

        //takes forever and eats all the memory it could find, starting with depth 6
        for (int i=1; i < n; i++) {
            Fingerprinter fp = new Fingerprinter(1024,i);

// here add code to configure atom types - and make sure to set atomic numbers as well,
//because perceiveAtomType() method doesn't set them, and they are null after JNIInchi

            long now = System.currentTimeMillis();
            BitSet bitset = fp.getFingerprint(mol);
            System.out.println(String.format("Atoms\t%d\tBonds\t%d\tFP Depth\t%d\tElapsed time\t%d\tBitset\t%s",
             * depth 1 : 188 ms
             * depth 2 : 291 ms
             * depth 3 : 525 ms
             * depth 4 : 1474 ms
             * depth 5 : 9147 ms
             * depth 6 : 71634 ms


  • Not saying that this should not be added at all, but this is exactly why I wrote they hybridization fingerprinter... it does not use any aromaticity, and thus does not use any ring finder.

    Actually, how about just removing the original fingerprinter? We already have enough people shooting themselves in the foot...

  • The problem is likely not the rings finder (it does time out if involved). None of the fingerprinter classes complete with these compounds for one or another reason.

  • OK, if it applies to HybridizationFingerprinter, then indeed it is not the time out, but something more annoying. This needs exploring...

    But then we should move this to the bug tracker... or maybe file a separate bug report there?

  • John May
    John May

    Can't look at all the fingerprint classes ATM but for the basic Fingerprinter I'm hedging my bets on 'getPathsOfLengthUpto'. From the few molecules I could load they looked very interconnected and I would imagine would take a long time to enumerate all paths of length 8 for example. I'm not sure if the other fingerprinters use the path finder but doing some profiling could show where the problem is.

    Just checking but these structures are what you expect..? One I opened had a carbon with 14 bonds and some hydrogens with 4 - very Toxic i guess :-).

  • John, I am guessing more or less the same, path enumeration cost for some reason increases exponentially in the Fingerprinter.

    The structures are user queries, and for the record, the InChI library is parsing them just fine.

  • Asad

    Dear Nina and Developers,

    The problem you have highlighted is a genuine one and I came across this bottleneck last year too!
    In order to resolve this issue I have made a minor change in the concept of the path based fingerprints.

    I have replaced the PathTool.getPathOfLength this by shortest path between atoms and its works very well.

    The reasoning is very simple as the number of bond and atoms will increase the complexity will give rise to exponential runtime.
    The shortest path will overcome this bottle neck without loosing the connectivity information.

    I have also tweaked few aspects of the present Fingerprinter in the CDK to take care of rings etc.

    Here is the code https://github.com/asad/ShortestPathMoleculeFingerprinter

    The fingerprints from Nina's query are also attached.




  • Egon, not sure why we should remove the original fingerprinter if we have not the slighted idea about what the bug is!
    There is no clear indication what the bug is and looking at the implementation, I cannot see why there should be a combinatorial explosion. I have not been able so far to depict the molecules. As John points out, they are "special cases" and I would love to see table with those molecules before making any judgement.

  • Structures and fingerprints upt to depth 6

  • Asad, thanks for confirming the problem is a genuine one. Pity I was not aware of your experience, would have saved some time!

    Christoph, I agree these structures are special cases, there are indeed atoms with unusual connectivity. Generating structure diagrams unfortunately fails, I haven't investigated the reason. The attached structures_and_fp_depth6.zip file contains SDF file and Java code building the structures (by CDKSourceCodeWriter ), as well as statistics for Fingerprinter, ExtendedFingerprinter and HybridizationFingerprinter up to depth 6 for all example structures.

    However, just by Google search by InChI, these structures can be found in the Chempound repository. Links and statistics at https://gist.github.com/3192020

    Regardless of the structure correctness, once these InChIs are in the wild, they may come as queries or input to any running cheminformatics system. These are not the only problem structures in our logs, just most difficult ones. They are successfully parsed by the InChI library, and while CDK atom typing rightly fails for most of the atoms, it is not a reason to automatically discard the structures. I've suggested a timeout, because it is the easiest way to protect a system from hanging and consuming resources indefinitely. A better approach by checking chemical correctness would be most useful.

  • Christoph,

    Egon, not sure why we should remove the original fingerprinter if we have
    not the slighted idea about what the bug is!

    The time out is not about a bug, I guess. It is about the algorithm just taking long. It could be a bug, but I have no seen any argument for that.

    There is no clear indication what the bug is and looking at the
    implementation, I cannot see why there should be a combinatorial explosion.

    The original Fingerprinter uses 'aromaticity'... that involves ring finding. Now, the newer CDK aromaticity perception code does not try all ring combinations, but as you know ring finding has been biting us many, many times. That is on top of the situation that longer paths just give more possibilities... I think that goes combinatorial.

    The Fingerprinter is reincarnated in the HybridizationFingerprinter; it is the same code, except that it substitutes 'aromaticity' by 'delocalized' (or sp2, to be precise)... So, removing the Fingerprinter class would not be the same as removing the original fingerprinter code.

  • John May
    John May

    • Request for close -

    Reason: Asad has implemented the shortest path fingerprinter to fix the provided issues

  • John May
    John May

    I will double check these molecules but the PathTools now checks if too many paths are being generated.

  • John May
    John May

    • Group: -->
  • John May
    John May

  • John May
    John May

    Should be fixed - need to confirm.