(no subject)

Page 1.0 of 1.6
  • Egon Willighagen

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

     
  • Nina Jeliazkova

    Nina Jeliazkova - 2012-07-21

    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.

     
  • Egon Willighagen

    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 - 2012-07-21

    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 :-).

     
  • Nina Jeliazkova

    Nina Jeliazkova - 2012-07-21

    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

    Asad - 2012-07-23

    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.

    git://gist.github.com/3161698.git

    Best,

    Asad

     
  • Christoph Steinbeck

    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.

     
  • Nina Jeliazkova

    Nina Jeliazkova - 2012-07-28

    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.

     
  • Egon Willighagen

    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 - 2012-10-23
    • Request for close -

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

     
  • John May

    John May - 2013-10-22

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

     
  • John May

    John May - 2013-10-22
    • Group: -->
     
  • John May

    John May - 2013-12-29
     
  • John May

    John May - 2013-12-29

    Should be fixed - need to confirm.

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:

JavaScript is required for this form.





No, thanks