[Cdk-devel] [ cdk-Patches-3374133 ] KEGG Atom type patches

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

Patches item #3374133, was opened at 2011-07-21 19:58
Message generated for change (Comment added) made by asadrahman
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3374133&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: cdk-1.4.x
Group: Needs Review
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: gilleain maclean torrance (gilleain)
Assigned to: Egon Willighagen (egonw)
Summary: KEGG Atom type patches

Initial Comment:
46 element-level patches. Each patch either promotes an existing atom type to a new method, or adds a new method for an element that wasn't already handled.

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

Comment By: Asad (asadrahman)
Date: 2011-08-17 06:25

Message:
>Hg doesn't look good at this moment either. It has this code:

I have fixed the Hg and the new patch is in the tracker.

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

Comment By: Asad (asadrahman)
Date: 2011-08-17 06:24

Message:
>N: needs more information on the atom types; the two new N atom types both
>violate the octet rule, which I cannot see why they should

Okay I will check it.

>S: causes a regression in the CDKAtomTypeMatcherSMILESTest

Well I think the regression is valid. I am not sure if this molecule is
valid.

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

Comment By: Egon Willighagen (egonw)
Date: 2011-08-16 17:01

Message:
N: needs more information on the atom types; the two new N atom types both
violate the octet rule, which I cannot see why they should

S: causes a regression in the CDKAtomTypeMatcherSMILESTest

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

Comment By: Egon Willighagen (egonw)
Date: 2011-08-16 15:21

Message:
Cleanup is needed for these atom types:

Al: the patch removes properties for existing atom types
Ru: not the right formal charge in one of the tests (but I also question a
Ru10+ atom type in general... are you kidding?)

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

Comment By: Egon Willighagen (egonw)
Date: 2011-08-09 21:49

Message:
I reviewed and approved this afternoon Ag, Au, Ba, Be, and Cr.

The aluminum patches needs checking as it currently changes properties of
other, existing atom types. Accidental, on purpose? If latter, split out
and file a bug report.

Hg doesn't look good at this moment either. It has this code:

+            if (neighbors == 1) {
+                IAtomType type = getAtomType("Hg.plus");
+                if (isAcceptable(atom, atomContainer, type)) return
type;
+            }
+            IAtomType type = getAtomType("Hg.plus");
+            if (isAcceptable(atom, atomContainer, type)) return type;

So, if it has one connection or more, it's the same Hg.plus atom type.
That's not right.

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

Comment By: Egon Willighagen (egonw)
Date: 2011-08-08 08:31

Message:
Nimish replied about the Cl.2 atom type I asked about ("I'll reject Cl for
now. Please check the hybridization, but sp3 cannot contain two sigma
bonds, a pi bond, and two lone pairs."):

------------------------------------------------------------
I went through the query related to the Atom type Cl.2

Actually Cl central atom in Chlorite ion ClOO- has only 10 electrons
(neutral) while the case of MolBase (Cl atom has negative charge)
hence 12 electrons which is completely different atom type.

Reference KEGG C01486.mol and
http://www.chemspider.com/Molecular-Formula/ClNaO2
------------------------------------------------------------

I'm good with this explanation.

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

Comment By: Egon Willighagen (egonw)
Date: 2011-08-08 00:14

Message:
OK, I just reviewed five atom types: In, Cd, Pu, Th look fine to me, though
we really must try to figure out the lone pairs for those atoms.

Fe failed for me, causing two failing unit tests. That one I will need to
look at later again.

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

Comment By: Egon Willighagen (egonw)
Date: 2011-08-07 23:54

Message:
I wrote on July 23 to Gilleain:

>> existing : Se

I get two failing unit tests in CDKAtomTypeMatcherTest.

>> Mn Si As

All three applied

>> S N

Doing these later.

>> Na

Applied

>> Ca

OK, the history is now so different from what is in the patch, this
patch no longer applies :(

>> Ge

Not looked at yet.

>> I Cl

I is approved.

I'll reject Cl for now. Please check the hybridization, but sp3 cannot
contain two sigma bonds, a pi bond, and two lone pairs.

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

Comment By: Egon Willighagen (egonw)
Date: 2011-08-04 15:53

Message:
An initial few atom types from this patch already made it into CDK 1.4.1.
If you want to help review the patches, please do so. But as soon as you
finished one atom type, please send the signed-off patch to me and Rajarshi
directly, so that it can immediately be applied.

I replied someone earlier today:
-------------------------------------------------
> what is the status of Gilleain's reformatted atom type patch. I know you
have more than enough to do.

Several have been applied already. Last week I had travel days, and
this week is catching up with work... See Jul 23:

https://github.com/cdk/cdk/commits/cdk-1.4.x

Make sure you're local version of the cdk-1.4.x branch is up to date.

> I understand that this has been reformatted into individual patches?

Yes, see below.

> Can we help to expedite the inclusion?

Absolutely! (You're the first to ask...)

The zip file with patches can be found here:

http://sourceforge.net/tracker/?func=detail&aid=3374133&group_id=20024&atid=320024

The patches should be applied to the cdk-1.4.x branch with 'git am -3
--ignore-whitespace' ... skill those that do not immediately work...
some will cleanly apply, others need a bit of work.

Then check the patch with 'git show' and check if you believe the atom
type is reasonable... also check if the number of lone pairs and
double bonds are defined (they must have a good excuse not to list
those, as we need that for various CDK algorithms). And do a general
peer review of the code.

Then, make sure nothing gets broken, by running:

ant clean dist-all test-dist-all
ant -Dmodule=core test-module

Particularly focus on the unit test.

If you are happy with all, run 'git commit --amend --signoff', and
create a new patch with 'git format-patch -1' and send that
0001-*.patch file to Rajarshi *and* me by email.
----------------------------------------------------

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

Comment By: Asad (asadrahman)
Date: 2011-07-27 10:54

Message:
Hi,

I have added a pdf file which contains information about atom types which
were fixed/updated. It also contains remark(s) and reference molecules for
each atom type for your perusal.

https://github.com/downloads/asad/cdk/Curated%20Atom%20type%20patches.pdf

Hope this helps.

Many thanks
Asad

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

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