Re: [Cdk-devel] NumericalSurface returns different results after rigid alignment

[John M. <joh...@gm...>]

In that case... temporarily replace this function:

https://github.com/cdk/cdk/blob/master/descriptor/qsarmolecular/src/main/java/org/openscience/cdk/geometry/surface/NeighborList.java#L97-L136

With the naïve n^2, i.e.

for (Atom a : atoms()){
>   for (Atom b : atoms()) {
>     if (a != b && dist(a, b) < threshold) {
>     }
>  }
> }


The hashing algorithm you have there is optimal but the simple one would
help prove correctness.

John

P.S. Roger gave a talk
<https://github.com/rdkit/UGM_2013/blob/master/Presentations/Sayle.RDKitGems.pdf>
on these algorithms at an RDKit meeting, a similar method is needed for the
rebonding of PDB files. CDK currently uses binary space partitioning to do
it but actually this is worse case for long strings of connected things
(i.e. peptides) and hashing cubes is much better.

On 20 February 2017 at 14:52, Rajarshi Guha <raj...@gm...> wrote:

> You're good! Stared at this for an hour last night and missed it :(
>
> Yes, the else clause should be adding the atom index. With that the
> surface areas are much more similar - 411.00944 vs 410.45645 - but not
> identical as I would expect they should be.
>
> You're probably right about the order of operations. Also probably should
> look at Tesselate
>
> On Mon, Feb 20, 2017 at 9:33 AM, John Mayfield <
> joh...@gm...> wrote:
>
>> Not tested but I think the problem is here: https://github.com/cdk/c
>> dk/blob/master/descriptor/qsarmolecular/src/main/java/org/
>> openscience/cdk/geometry/surface/NeighborList.java#L55-L61
>>
>> Shouldn't the else also add the atom to the list?
>>
>> A shorter, more efficient and less error prone idiom is:
>>
>> List<IAtom> atoms = this.boxes.get(key);
>>> if (atoms == null)
>>>   this.boxes.put(key, atoms = new ArrayList<>());
>>> atoms.add(atom);
>>
>>
>> Notice we do a single get, then iff the value was null we create a new
>> one and add it. We always add the atom to the atoms array.
>>
>> I can see that some people may find the assignment in the put confusing
>> (in C++ the put would do a copy so the atoms.add looks like a no-op).
>>
>> John
>>
>> On 20 February 2017 at 13:57, Rajarshi Guha <raj...@gm...>
>> wrote:
>>
>>> Here's some sample code https://gist.github.com/r
>>> ajarshi/86389392173191ee15cef7b76d994ae5 and the SD files are available
>>> at https://www.dropbox.com/sh/xu548wx8brqx9z9/AABloecWNrSXAE
>>> cwud7ZSV-Oa?dl=0
>>>
>>> Poking around I found our that the neighbor lists for a given atom
>>> differ between the aligned and non-aligned cases.
>>>
>>> On Mon, Feb 20, 2017 at 4:59 AM, John Mayfield <
>>> joh...@gm...> wrote:
>>>
>>>> Have you got the molecule/usage? My money is on either order of
>>>> operations / Tesselate.java.
>>>>
>>>> John
>>>>
>>>> On 20 February 2017 at 02:54, Rajarshi Guha <raj...@gm...>
>>>> wrote:
>>>>
>>>>> Hi, a user pointed out a bug in the CPSA descriptor that is generating
>>>>> different results for the same molecule after a rigid alignment.
>>>>>
>>>>> It turns out that the bug arises due to NumericalSurface giving
>>>>> different total surface area values for the molecule before alignment and
>>>>> after alignment.
>>>>>
>>>>> I confirmed that the relative positions of the atoms are identical in
>>>>> the two versions of the molecule (pairwise distance matrix is the same to
>>>>> four decimal places).
>>>>>
>>>>> Does anybody have any immediate ideas about why a rigid alignment
>>>>> would lead to a difference in the computed surface area?
>>>>>
>>>>> --
>>>>> Rajarshi Guha | http://blog.rguha.net
>>>>> NIH Center for Advancing Translational Science
>>>>>
>>>>> ------------------------------------------------------------
>>>>> ------------------
>>>>> Check out the vibrant tech community on one of the world's most
>>>>> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
>>>>> _______________________________________________
>>>>> Cdk-devel mailing list
>>>>> Cdk...@li...
>>>>> https://lists.sourceforge.net/lists/listinfo/cdk-devel
>>>>>
>>>>>
>>>>
>>>
>>>
>>> --
>>> Rajarshi Guha | http://blog.rguha.net
>>> NIH Center for Advancing Translational Science
>>>
>>
>>
>
>
> --
> Rajarshi Guha | http://blog.rguha.net
> NIH Center for Advancing Translational Science
>