[Cdk-devel] [ cdk-Feature Requests-1678346 ] GENMDeterministicGenerator problem

[SourceForge.net <no...@so...>]

Feature Requests item #1678346, was opened at 2007-03-11 16:31
Message generated for change (Comment added) made by egonw
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=370024&aid=1678346&group_id=20024

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
>Status: Closed
Priority: 5
Private: No
Submitted By: AlexHenderson (alexhenderson)
Assigned to: Christoph Steinbeck (steinbeck)
Summary: GENMDeterministicGenerator problem

Initial Comment:
Hi

There appears to be a bug (perhaps 2) in the GENMDeterministicGenerator unit. 

The number of structures quoted for CH3PO4 and CF3PO4 are different at 32 and 63 respectively. 

Since both hydrogen and fluorine are monovalent one would expect that they should have the same number of possible structures. Of course these might not be structures that exist in the 'real-world', but that's a different issue. 

See here for example code, CH3PO4 and CF3PO4 structures -  http://www.geordiesoft.co.uk/cdkbug/

It would appear that the hydrogen version has structures missing. For example the first fluorine structure has no equivalent in the hydrogen list. 

A possible second bug is that both sets have structures that do not conform to the molecular formula. For example see the last structure in the hydrogen list (or the fluorine list). This requires 5 hydrogen atoms to keep the valence balanced, but the molecular formula only has 3. 

Tested on...

    * Windows XP
    * NetBeans 5.5
    * Java 1.5 JDK
    * cdk-0.99.1.jar (or cdk-20060714.jar)

Thanks, 
Alex


----------------------------------------------------------------------

>Comment By: Egon Willighagen (egonw)
Date: 2008-08-13 07:58

Message:
Logged In: YES 
user_id=25678
Originator: NO

The code did not lend itself for extension, and was buggy too. It was
recently removed from trunk and cdk1.0.x/

----------------------------------------------------------------------

Comment By: Nobody/Anonymous (nobody)
Date: 2007-06-27 05:04

Message:
Logged In: NO 

ALSO CH4


----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2007-04-15 11:06

Message:
Logged In: YES 
user_id=25678
Originator: NO

The limited set of fragments is being attempted to extend. The observed
behavior, however, is the expected behavior, so making this a Feature
Request for now.

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2007-03-21 16:04

Message:
Logged In: YES 
user_id=25678
Originator: NO

BTW, regarding patches, those are most welcome, and I will review them
anyway.

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2007-03-21 16:03

Message:
Logged In: YES 
user_id=25678
Originator: NO

Hi Alex,

copy you on that. I just had a look at it, because the possibility to add
fragments is quite interesting. But the code is indeed impossible to
understand. I tried adding the phosphate group, which is not in the list,
and explains the behavior you found.

I suggest that we add a large set of JUnit tests first (see
cdk.test.structgen.deterministic.GENMDeterministicGeneratorTest), so that
we can b reasonably sure not to break anything.

I can confirm the problems with Si, and added some tests for that. Making
the patch you propose does not help, however.

To be continued.

----------------------------------------------------------------------

Comment By: AlexHenderson (alexhenderson)
Date: 2007-03-12 16:00

Message:
Logged In: YES 
user_id=1233244
Originator: YES

Hi,
Another (small) problem, while you're in there, is that the BasicUnits of
Si are not being set. Instead they are a copy of the C BasicUnits. Lines
304-306 should be something like 
maxNumberOfBasicUnit[7]=molecularFormula[7];//SiH3
maxNumberOfBasicUnit[8]=molecularFormula[7];//SiH2
maxNumberOfBasicUnit[9]=molecularFormula[7];//SiH

I was having some difficulty understanding what the module was doing
because it's been optimised so much with heavy use of arrays. Unfortunately
the indexes into these arrays change between functions so that, for
example, MolecularFormula[6] means phosphorus, but BasicUnit[6] means
something else. I was tempted to have a go at re-writing it using defined
constants, but I'm not sure my Java is up to it (yet!). 

Cheers, 
Alex


----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2007-03-12 14:13

Message:
Logged In: YES 
user_id=25678
Originator: NO

I checked the code and the P-with-four-neighbors is not in the fragment
list, causing it to fail.


----------------------------------------------------------------------

Comment By: Christoph Steinbeck (steinbeck)
Date: 2007-03-11 22:19

Message:
Logged In: YES 
user_id=54358
Originator: NO

This looks very bad indeed. We'll have a look at it. The unsaturated
phosphorus in found in my of the fluorine structures is also very
disturbing.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=370024&aid=1678346&group_id=20024