#1291 AtomContainerSet.sortAtomContainers() causes NullPointerExceptoin

cdk-1.4.x
closed
nobody
None
1
2013-03-11
2013-01-25
No

The impelementations of AtomContainerSet.sortAtomContainers() use

Arrays.sort(atomContainers, comparator);

However, atomContainers.length can be greater than the actual AtomContainerSet size, i.e. atomContainerCount. After sorting (depending on the comparator) you can end up with the null elements of the atomContainers array being interspersed through the array.

I believe sortAtomContainers() should use

Arrays.sort(atomContainers, 0, atomContainerCount, comparator);

thus restricting the sorting to the actual members of the set.

I am seeing this bug manifesting as NPEs when rendering structures:

Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException
at org.openscience.cdk.RingSet.getRings(RingSet.java:72)
at org.openscience.cdk.tools.manipulator.RingSetManipulator.getHeaviestRing(RingSetManipulator.java:154)
at org.openscience.cdk.renderer.generators.BasicBondGenerator.generate(BasicBondGenerator.java:276)
at org.openscience.cdk.renderer.generators.BasicBondGenerator.generate(BasicBondGenerator.java:262)
at org.openscience.cdk.renderer.generators.BasicBondGenerator.generate(BasicBondGenerator.java:63)
at org.openscience.cdk.renderer.AbstractRenderer.generateDiagram(AbstractRenderer.java:119)
at org.openscience.cdk.renderer.AtomContainerRenderer.paint(AtomContainerRenderer.java:203)

Related

Bugs: #1291

Discussion

  • John May

    John May - 2013-01-25

    Hi Chris,

    The patch is here: http://sourceforge.net/p/cdk/patches/600/

    It could be fixed with the sub-array sort but actually in this case the comparator is also the issue. This is actually the second time it's been reported in a week.

    Unfortunately only Egon/Rajarshi can push changes to the repo.

    Thanks,
    J

     
  • Chris Pudney

    Chris Pudney - 2013-01-28

    Thanks John.

    I'd suggest both need fixing.

    The sub-array sort makes the code more robust; Comparators that don't properly handle null AtomContainers will not result in NPEs.

    Additionally, it's semantically correct - we should only sort the members of the Set, not the empty portion of the storage array.

    And it'll be (marginally) quicker ;-)

     
  • John May

    John May - 2013-01-28

    Yep, I'll include that in the patch. Of course it should really just use a List and not have to concern it's self with array resizing :).

    Thanks

     
  • Chris Pudney

    Chris Pudney - 2013-01-29

    Looking more closely at AtomContainerSet I see there's also a multipliers field.

    I think sorting the Set is going to break the multipliers field because the order of the elements in atomContainers will no longer correspond to the ordering of multipliers.

    Could a HashMap be used to provide the multipliers rather than an array? Then the order of atomContainers can be changed arbitrarily.

     
  • Chris Pudney

    Chris Pudney - 2013-01-29

    Could a HashMap be used to provide the multipliers rather than an array?

    Need to consider the semantics of adding an AtomContainer to an AtomContainerSet more than once...

     
  • John May

    John May - 2013-01-29

    Yep, looks like the sorting was already broken unfortunately.

    Need to consider the semantics of adding an AtomContainer to an AtomContainerSet more than once

    True, although technically the name Set implies you can't do this... although you can :(.

    From my understanding, this was originally created for reactions (hence the multiplies) - and then later adapted for just holding multiple atom containers (with the sort). Of course storing the multiplies separate from the atom containers causes head aches as these two things which are associative are associated.

    currently the storage looks like this:

    coefficients = (c1, c2, c3, ... cn)
    molecules    = (m1, m2, m3, ... mn)
    

    a better approach is to group them together

    combined = ((c1, m1), (c2, m2), (c3, m3), ... (cn, mn))
    

    this also makes more sense in reactions. You can then change what the 'participant' contains. Generally you'd only want molecule and coefficient but in biochemical reactions it is also important to include the compartment. This second scheme is a bit more flexible.

    I'll see if can fix it without any major changes first.

    J

     
  • John May

    John May - 2013-01-29

    Another issue is the class is serializable... looks like this won't be clean.

     
  • John May

    John May - 2013-01-29

    Okay done :-). I thought about it and was pretty easy. It's not the "right" way but it keeps the current fields. Basically you sort the indices based on the comparator and then read off the sorted values.

    public void sortAtomContainers(final Comparator<IAtomContainer> comparator) {
    
            // need to use boxed primitives as we can't customise sorting of int primitives
            Integer[] indexes = new Integer[atomContainerCount];
            for(int i = 0; i < indexes.length; i++)
                indexes[i] = i;
    
            // proxy the index comparison to the atom container comparator
            Arrays.sort(indexes, new Comparator<Integer>(){
                @Override public int compare(Integer o1, Integer o2) {
                    return comparator.compare(atomContainers[o1], atomContainers[o2]);
                }
            });
    
            // copy the original arrays (we could modify in place with swaps but this is cleaner)
            IAtomContainer[] containersTmp  = Arrays.copyOf(atomContainers, indexes.length);
            Double[]         multipliersTmp = Arrays.copyOf(multipliers, indexes.length);
    
            // order the arrays based on the order of the indices
            for(int i = 0; i < indexes.length; i++) {
                atomContainers[i] = containersTmp[indexes[i]];
                multipliers[i] = multipliersTmp[indexes[i]];
            }
    
        }
    
     
  • Chris Pudney

    Chris Pudney - 2013-01-30

    Clever solution.

     
  • Egon Willighagen

    John, I have applied several of the patches you lined up... see the cdk-1.4.x branch.

     
  • John May

    John May - 2013-01-30

    Okay thanks,

    Were you able to do the last one (after the logging patch): https://github.com/johnmay/cdk/commit/0d59bc4a77f7ad0fd4778d1d143cc32fcc675e43

    This fixes the sorting of the multiples (with unit tests).

    Thanks,
    J

    On 30 Jan 2013, at 12:25, Egon Willighagen egonw@users.sf.net wrote:

    John, I have applied several of the patches you lined up... see the cdk-1.4.x branch.

    [bugs:#1291] AtomContainerSet.sortAtomContainers() causes NullPointerExceptoin

    Status: open
    Created: Fri Jan 25, 2013 09:38 AM UTC by Chris Pudney
    Last Updated: Wed Jan 30, 2013 02:47 AM UTC
    Owner: nobody

    The impelementations of AtomContainerSet.sortAtomContainers() use

    Arrays.sort(atomContainers, comparator);

    However, atomContainers.length can be greater than the actual AtomContainerSet size, i.e. atomContainerCount. After sorting (depending on the comparator) you can end up with the null elements of the atomContainers array being interspersed through the array.

    I believe sortAtomContainers() should use

    Arrays.sort(atomContainers, 0, atomContainerCount, comparator);

    thus restricting the sorting to the actual members of the set.

    I am seeing this bug manifesting as NPEs when rendering structures:

    Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException
    at org.openscience.cdk.RingSet.getRings(RingSet.java:72)
    at org.openscience.cdk.tools.manipulator.RingSetManipulator.getHeaviestRing(RingSetManipulator.java:154)
    at org.openscience.cdk.renderer.generators.BasicBondGenerator.generate(BasicBondGenerator.java:276)
    at org.openscience.cdk.renderer.generators.BasicBondGenerator.generate(BasicBondGenerator.java:262)
    at org.openscience.cdk.renderer.generators.BasicBondGenerator.generate(BasicBondGenerator.java:63)
    at org.openscience.cdk.renderer.AbstractRenderer.generateDiagram(AbstractRenderer.java:119)
    at org.openscience.cdk.renderer.AtomContainerRenderer.paint(AtomContainerRenderer.java:203)

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

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

     

    Related

    Bugs: #1291

  • Egon Willighagen

    No, not seen that one yet...

    Egon

    On Wed, Jan 30, 2013 at 2:12 PM, John May jwmay@users.sf.net wrote:

    Okay thanks,

    Were you able to do the last one (after the logging patch):
    https://github.com/johnmay/cdk/commit/0d59bc4a77f7ad0fd4778d1d143cc32fcc675e43

    This fixes the sorting of the multiples (with unit tests).

    Thanks,
    J

    On 30 Jan 2013, at 12:25, Egon Willighagen egonw@users.sf.net wrote:

    John, I have applied several of the patches you lined up... see the
    cdk-1.4.x branch.

    [bugs:#1291] AtomContainerSet.sortAtomContainers() causes
    NullPointerExceptoin

    Status: open
    Created: Fri Jan 25, 2013 09:38 AM UTC by Chris Pudney
    Last Updated: Wed Jan 30, 2013 02:47 AM UTC
    Owner: nobody

    The impelementations of AtomContainerSet.sortAtomContainers() use

    Arrays.sort(atomContainers, comparator);

    However, atomContainers.length can be greater than the actual
    AtomContainerSet size, i.e. atomContainerCount. After sorting (depending on
    the comparator) you can end up with the null elements of the atomContainers
    array being interspersed through the array.

    I believe sortAtomContainers() should use

    Arrays.sort(atomContainers, 0, atomContainerCount, comparator);

    thus restricting the sorting to the actual members of the set.

    I am seeing this bug manifesting as NPEs when rendering structures:

    Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException
    at org.openscience.cdk.RingSet.getRings(RingSet.java:72)
    at
    org.openscience.cdk.tools.manipulator.RingSetManipulator.getHeaviestRing(RingSetManipulator.java:154)
    at
    org.openscience.cdk.renderer.generators.BasicBondGenerator.generate(BasicBondGenerator.java:276)
    at
    org.openscience.cdk.renderer.generators.BasicBondGenerator.generate(BasicBondGenerator.java:262)
    at
    org.openscience.cdk.renderer.generators.BasicBondGenerator.generate(BasicBondGenerator.java:63)
    at
    org.openscience.cdk.renderer.AbstractRenderer.generateDiagram(AbstractRenderer.java:119)
    at
    org.openscience.cdk.renderer.AtomContainerRenderer.paint(AtomContainerRenderer.java:203)

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

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


    [bugs:#1291] AtomContainerSet.sortAtomContainers() causes
    NullPointerExceptoin

    Status: open
    Created: Fri Jan 25, 2013 09:38 AM UTC by Chris Pudney
    Last Updated: Wed Jan 30, 2013 12:25 PM UTC
    Owner: nobody

    The impelementations of AtomContainerSet.sortAtomContainers() use

    Arrays.sort(atomContainers, comparator);

    However, atomContainers.length can be greater than the actual
    AtomContainerSet size, i.e. atomContainerCount. After sorting (depending on
    the comparator) you can end up with the null elements of the atomContainers
    array being interspersed through the array.

    I believe sortAtomContainers() should use

    Arrays.sort(atomContainers, 0, atomContainerCount, comparator);

    thus restricting the sorting to the actual members of the set.

    I am seeing this bug manifesting as NPEs when rendering structures:

    Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException
    at org.openscience.cdk.RingSet.getRings(RingSet.java:72)
    at
    org.openscience.cdk.tools.manipulator.RingSetManipulator.getHeaviestRing(RingSetManipulator.java:154)
    at
    org.openscience.cdk.renderer.generators.BasicBondGenerator.generate(BasicBondGenerator.java:276)
    at
    org.openscience.cdk.renderer.generators.BasicBondGenerator.generate(BasicBondGenerator.java:262)
    at
    org.openscience.cdk.renderer.generators.BasicBondGenerator.generate(BasicBondGenerator.java:63)
    at
    org.openscience.cdk.renderer.AbstractRenderer.generateDiagram(AbstractRenderer.java:119)
    at
    org.openscience.cdk.renderer.AtomContainerRenderer.paint(AtomContainerRenderer.java:203)


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

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

    --
    Dr E.L. Willighagen
    Postdoctoral Researcher
    Department of Bioinformatics - BiGCaT
    Maastricht University (http://www.bigcat.unimaas.nl/)
    Homepage: http://egonw.github.com/
    LinkedIn: http://se.linkedin.com/in/egonw
    Blog: http://chem-bla-ics.blogspot.com/
    PubList: http://www.citeulike.org/user/egonw/tag/papers

     

    Related

    Bugs: #1291

  • John May

    John May - 2013-03-11
    • status: open --> closed
     
  • John May

    John May - 2013-03-11

    resolved