#600 AtomContainerComparatorBy2DCenter Null Default

Needs_Review
closed
nobody
None
cdk-1.4.x
9
2013-02-08
2012-12-19
No

The default Point2D value for a null container in the Comparator is Double.MIN_VALUE. This results in non-null containers getting pushed down in the array. For the case of a single container in an array, that means:

Because of the center(IAtomContainer container) method, the return is -1. Hence, the non-null container that used to be at index 0 in the array is resorted to the last position of the array. Consequently trying to get the container at position 0 somewhere later in the code fails.

The patch inverts that behavior by ensuring that null always ends up higher in the array than non-null container.

1 Attachments

Discussion

  • John May

    John May - 2012-12-19

    My bad on this one :D... always forgot the sort goes from low to high.

     
  • Egon Willighagen

    Who reviewed this?!? ... ... ...

     
  • John May

    John May - 2012-12-19

    You did? Didn't you?

     
    • Egon Willighagen

      I know ... so, my point was: it's not just your bad, but mine too!

       
      • John May

        John May - 2012-12-19

        Hehe oh right. Well I still wrote it wrong in the first place. :)

         
  • John May

    John May - 2012-12-19
    • branch: master --> cdk-1.4.x
    • priority: 1 --> 5
     
  • John May

    John May - 2012-12-19

    Also on 1.4.x

     
  • John May

    John May - 2012-12-20
    • priority: 5 --> 9
     
  • John May

    John May - 2012-12-20

    Sorry I thought the priority was obvious.
    - This results in non-null containers getting pushed down in the array. For the case of a single container in an array. - i.e. NPE when it's used as some code expects containers to be at index '0'.

     
    Last edit: John May 2012-12-20
  • Egon Willighagen

    Stephan, I would suggest to add a unit type, and the description of the field with "minimum" is awkward now... can that be updated?

     
  • Egon Willighagen

    Please make things available as git branch with all things that should go in, when you got the unit test worked out.

     
  • John May

    John May - 2013-01-28

    Fixes/Units tests are on master+

    The four commits span, 16f404cf0f - d78d95cf0f inclusive.

     
  • John May

    John May - 2013-01-28

    Oh and they're on master but do apply to cdk-1.4.x without problem

     
    • John May

      John May - 2013-01-28

      Oh, maybe not. I forgot Mockito isn't on 1.4.x

       
  • Egon Willighagen

    :)

    OK, please rebase. I will look at it tonight, and if OK, do the 1.4.17 release.

     
  • John May

    John May - 2013-01-28

    Okay the rebased commits for 1.4.x are on: https://github.com/johnmay/cdk/commits/cdk-1.4.x+. Please use the other ones for master as the unit tests are cleaner.

    The last commit relates to a different patch on the AtomContainerAtomPermutator. I thought you'd pulled that but I can't find it on the logs.

     
  • John May

    John May - 2013-01-29

    I've updated both master+ and cdk-1.4.x+ branches with another fix. As Chris P points out on the bug the multipliers were never sorted with the atom containers - therefore they would go out of sync.

     
  • John May

    John May - 2013-02-08

    master/6b8d8ca2b2

     
  • John May

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

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

Sign up for the SourceForge newsletter:





No, thanks