From: SourceForge.net <no...@so...> - 2011-08-16 14:21:29
|
Patches item #3374133, was opened at 2011-07-21 20:58 Message generated for change (Comment added) made by egonw 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: Egon Willighagen (egonw) Date: 2011-08-16 16: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 22: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 09: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 01: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-08 00: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 16: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 11: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 |