Hi Nina and Egon
Thanks for your work and comments.
I attach two new java files for the FixBondOrdersTool and its test class. Both of these originated from Nina's patch but with the following changes:
* I have fixed all the problems Egon lists below (mostly relating to Javadoc). Javadoc generated by NetBeans looks fine to me now.
* I have made three changes to the source code. Firstly I noticed that the original class actually altered the molecule it
was passed. The new version starts by cloning the passed molecule, ultimately returning the kekulised clone.
* I have taken up Nina's suggestion of using the CDKHydrogenAdder in case the implicit hydrogen count is null (see getFreeValenciesForRingGroup for this and the next example)
* I have added code which explicitly checks for C- with Planar3 hybridisation in 5-membered rings and sets the implicitHydrogenCount appropriately.
* I have fixed the problems Egon lists
* I noticed that some of the tests were actually using the DeduceBondSystemTool and I have switched these over to FixBondOrdersTool
* I have removed the timeout test (as this isn't appropriate for FixBondOrdersTool)
* I removed the explicit addition of implicit hydrogen atoms (with the CDKHydrogenAdder) as this is done by the FixBondOrdersTool itself now (see above)
With these changes, FixBondOrdersTool now passes 11 out of 12 tests - the failure being xtestQuinone as expected.
I imagine you will want to test the new code before possibly making a new patch but I have done a fair amount of testing and
have convinced myself that no new regressions have been introduced (fingers crossed)!
Hope this is useful
From: Egon Willighagen [mailto:egon.willighagen@...]
Sent: 19 May 2012 11:18
To: Lawson Kevin GBJH; Nina Jeliazkova; cdk-devel
Subject: the FixBondOrdersTool patch
I am making some today today for the CDK, and just looked at your
patch. I was happy to hear about Nina's test results, and 1% error is
very much acceptable to me!
Nina created a patch, which I uploaded and left comments on. I cannot
accept it in the current form for cdk-1.4.x as I'd be excepting things
I asked others to correct, so it will not make the 1.4.10 release...
my comments are here:
I rely on Nina's analysis on the SD file for the algorithm, and
focused on the code quality. They're mostly small things, but I
haven't got time to day to fix those. Many of these things are
explained in more detail in my blog series:
If you have questions about my comments, please leave them as replies
to my comments... I'll get an email then. I fixed two issues with a
patch already, so make sure to check that.
Just to make clear, anyone can make patches to address the issues I
commented; if you want to see this excellent work applied soon, you
can help out!
Dr E.L. Willighagen
Department of Bioinformatics - BiGCaT
Maastricht University (http://www.bigcat.unimaas.nl/)
This message may contain confidential information. If you are not the designated recipient, please notify the sender immediately, and delete the original and any copies. Any use of the message by you is prohibited.