#1269 atom with nulled pos not placed by SDG, causing NPE

cdk-1.4.x
closed
nobody
None
7
2013-06-08
2012-10-27
Ralf Stephan
No

The primary symptom this shows is an NPE in Vector, caused by an atom with null 2d position field, given as part of a molecule to HydrogenPlacer.placeHydrogens2D. This is part of SDG.generateCoordinates(). The atom position is originally nulled in JCP's ControllerHub.cleanup() function in apparent anticipation that generateCoordinates() would fill all those nulls. I understand this promise is never made, rather a flag is set on the atoms, and this is checked. So, we don't know what the SDG promises exactly, and what to do instead of pos nulling to clean the molecule.

java.lang.NullPointerException
at javax.vecmath.Tuple2d.(Tuple2d.java:82)
at javax.vecmath.Vector2d.(Vector2d.java:86)
at org.openscience.cdk.layout.AtomPlacer.distributePartners(AtomPlacer.java:163)
at org.openscience.cdk.layout.HydrogenPlacer.placeHydrogens2D(HydrogenPlacer.java:112)
at org.openscience.cdk.layout.HydrogenPlacer.placeHydrogens2D(HydrogenPlacer.java:68)
at org.openscience.cdk.layout.StructureDiagramGenerator.generateExperimentalCoordinates(StructureDiagramGenerator.java:280)
at org.openscience.cdk.layout.StructureDiagramGenerator.generateExperimentalCoordinates(StructureDiagramGenerator.java:248)
at org.openscience.jchempaint.controller.ControllerHub.generateNewCoordinates(ControllerHub.java:1266)
at org.openscience.jchempaint.controller.ControllerHub.cleanup(ControllerHub.java:1227)
at org.openscience.jchempaint.action.CleanupAction.actionPerformed(CleanupAction.java:61)

The input in JCP is any SMILES containing a [C@H] then click the cleanup button.

Discussion

  • John May
    John May
    2012-10-27

    Hi Ralf,

    These lines look suspicious -

    https://github.com/egonw/cdk/blob/cdk-1.4.x/src/main/org/openscience/cdk/layout/HydrogenPlacer.java#L99-102

    If the atom isn't hydrogen but does have null coordinates it still gets placed.

    Also I need to look deeper but something that could help is ISPLACED flag to false (maybe you're doing this already). This is what determines which atoms are and aren't placed/unplaced: https://github.com/egonw/cdk/blob/cdk-1.4.x/src/main/org/openscience/cdk/layout/StructureDiagramGenerator.java#L778-792

    From your stack trace it looks like it the carbon with null coordinates…

    Could you attach the DEBUG log?

    J

    On 27 Oct 2012, at 18:16, Ralf Stephan rwst@users.sf.net wrote:

    bugs:1269 atom with nulled pos not placed by SDG, causing NPE

    Status: open Created: Sat Oct 27, 2012 05:16 PM UTC by Ralf Stephan Last Updated: Sat Oct 27, 2012 05:16 PM UTC Owner: nobody

    The primary symptom this shows is an NPE in Vector, caused by an atom with null 2d position field, given as part of a molecule to HydrogenPlacer.placeHydrogens2D. This is part of SDG.generateCoordinates(). The atom position is originally nulled in JCP's ControllerHub.cleanup() function in apparent anticipation that generateCoordinates() would fill all those nulls. I understand this promise is never made, rather a flag is set on the atoms, and this is checked. So, we don't know what the SDG promises exactly, and what to do instead of pos nulling to clean the molecule.

    java.lang.NullPointerException
    at javax.vecmath.Tuple2d.(Tuple2d.java:82)
    at javax.vecmath.Vector2d.(Vector2d.java:86)
    at org.openscience.cdk.layout.AtomPlacer.distributePartners(AtomPlacer.java:163)
    at org.openscience.cdk.layout.HydrogenPlacer.placeHydrogens2D(HydrogenPlacer.java:112)
    at org.openscience.cdk.layout.HydrogenPlacer.placeHydrogens2D(HydrogenPlacer.java:68)
    at org.openscience.cdk.layout.StructureDiagramGenerator.generateExperimentalCoordinates(StructureDiagramGenerator.java:280)
    at org.openscience.cdk.layout.StructureDiagramGenerator.generateExperimentalCoordinates(StructureDiagramGenerator.java:248)
    at org.openscience.jchempaint.controller.ControllerHub.generateNewCoordinates(ControllerHub.java:1266)
    at org.openscience.jchempaint.controller.ControllerHub.cleanup(ControllerHub.java:1227)
    at org.openscience.jchempaint.action.CleanupAction.actionPerformed(CleanupAction.java:61)

    The input in JCP is any SMILES containing a C@H then click the cleanup button.

    Sent from sourceforge.net because you indicated interest in https://sourceforge.net/p/cdk/bugs/1269/

    To unsubscribe from further messages, please visit https://sourceforge.net/auth/prefs/


    WINDOWS 8 is here.
    Millions of people. Your app in 30 days.
    Visit The Windows 8 Center at Sourceforge for all your go to resources.
    http://windows8center.sourceforge.net/
    join-generation-app-and-make-money-coding-fast/_______
    Cdk-bugs mailing list
    Cdk-bugs@lists.sourceforge.net
    https://lists.sourceforge.net/lists/listinfo/cdk-bugs

     
  • Ralf Stephan
    Ralf Stephan
    2012-10-28

    test comment

     
  • Ralf Stephan
    Ralf Stephan
    2012-10-28

    the UNSET flag is identical to null, this is not the (or Jcp's) problem

    24773 [AWT-EventQueue-0] DEBUG org.openscience.cdk.layout.HydrogenPlacer - Atom placement after procedure:
    24773 [AWT-EventQueue-0] DEBUG org.openscience.cdk.layout.HydrogenPlacer - Center atom O: (1.2990381056766584, 2.249999999999999)
    24773 [AWT-EventQueue-0] DEBUG org.openscience.cdk.layout.HydrogenPlacer - Now placing hydrogens at atom 3
    24773 [AWT-EventQueue-0] DEBUG org.openscience.cdk.layout.HydrogenPlacer - bondLength 1.5
    24773 [AWT-EventQueue-0] DEBUG org.openscience.cdk.layout.HydrogenPlacer - Atom placement before procedure:
    24773 [AWT-EventQueue-0] DEBUG org.openscience.cdk.layout.HydrogenPlacer - Center atom H: null

     
  • John May
    John May
    2012-10-28

    Have you got the "exact" SMILES you were using? Will try and recreate and work out what is going on

    Thanks,
    J

     
  • Ralf Stephan
    Ralf Stephan
    2012-10-28

    Quote above: "The input in JCP is any SMILES containing a [C@H]..." The actual strings given were '[C@H]' and 'O=C(O)[C@H](N)C' (L-alanine)

     
  • Ralf Stephan
    Ralf Stephan
    2012-10-28

    Test of mail forwarding. The previous comment wasn't seen for 1 hour.

     
  • John May
    John May
    2012-10-28

    Okay here's the problem lines: https://github.com/egonw/cdk/blob/cdk-1.4.x/src/main/org/openscience/cdk/layout/HydrogenPlacer.java#L64-69

    They have been commented out but I can tell why:

    [intrepid ~/workspace/github/johnmay/cdk]: git blame -L64,65     src/main/org/openscience/cdk/layout/HydrogenPlacer.java
    7c5c872c (egonw 2008-02-22 19:13:45 +0000 64) //                if (!atom.getSymbol().equals("H"))
    7c5c872c (egonw 2008-02-22 19:13:45 +0000 65) //                {
    

    unfortunately this is from svn - so the commit message is

    Merged the branch egonw/maintest: sets up src/main and src/test for splitting main library from unit t

    git-svn-id: https://cdk.svn.sourceforge.net/svnroot/cdk/trunk/cdk@10219 eb4e18e3-b210-0410-a6ab-dec725
    

    Unfortunately this is as far back as we can go

    [intrepid ~/workspace/github/johnmay/cdk]: git blame -L64,65 7c5c872c^ -- src/main/org/openscience/cdk/layout/HydrogenPlacer.java
    fatal: no such path src/main/org/openscience/cdk/layout/HydrogenPlacer.java in 7c5c872c^
    

    Simple solution is to add this conditional back in but there may be a reason why it was commented out in the first place.

    Egon might have some ideas and we may able to check the SVN log.

     
  • Ralf Stephan
    Ralf Stephan
    2012-11-14

    I've run a full testsuite with and without these lines. No difference appeared in the test results except for the test for the bug itself. I have also checked that a single H gets its implicit H; this is not affected. I therefore see no reason not to uncomment those lines.

    BTW, the bug preamble should read

    @Test (timeout=5000)
    

    because with the exception expected it will count a failure if there is NO exception.

     
  • John May
    John May
    2013-06-08

    • status: open --> closed
     
  • John May
    John May
    2013-06-08

    fixed