#600 AtomContainerComparatorBy2DCenter Null Default

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.

[John May]

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

[Egon Willighagen]

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

[John May]

You did? Didn't you?

[Egon Willighagen]

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

[John May]

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

[John May]

Also on 1.4.x

[John May]

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

[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?

[John May]

Including suggestions for Chris P and only sorting the sub-array.

[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]

Fixes/Units tests are on master+

The four commits span, 16f404cf0f - d78d95cf0f inclusive.

[John May]

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

[John May]

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]

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]

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]

master/6b8d8ca2b2