From: SourceForge.net <no...@so...> - 2012-01-22 14:11:08
|
Patches item #3474944, was opened at 2012-01-17 06:16 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3474944&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 Revision >Status: Closed >Resolution: Fixed Priority: 6 Private: No Submitted By: Rajarshi Guha (rajarshi) Assigned to: Rajarshi Guha (rajarshi) Summary: proper handling of pseudoatoms in MDL files Initial Comment: I was looking at a bug report where reading an SD file containing pseudoatoms (such as Gln, Ile etc) worked fine, but writing it out crashed with an exception. The reason for this is that during reading the atom with label Gln, is read in as a PseudoAtom with label "Gln" but symbol "R". The MDLV2000Writer sees the symbol R for this atom and assumes that it is an R group and then tries to parse the label to get an R-group number. Obviously for this case it fails. The problem is that *all* PseudoAtom objects get a symbol of R. While generally OK, this is not always correct for sdf/mol files. There are two possible solutions: 1. Modify PseudoAtom and related classes to allow one to specify the symbol as well as the label 2. Modify MDLV2000Reader & MDLV3000Reader to set the symbol for a pseudo atom explicitly to the label. This patch implements 2 ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2012-01-22 06:11 Message: I will try to keep doing merges... still worked the last time :) ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2012-01-22 05:46 Message: I agree with backporting to 1.4.x - I've applied and pushed them. Can you apply and push to master? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2012-01-22 02:45 Message: Rajarshi, if you agree with application to cdk-1.4.x, please find attached your patch signed off by me, and a patch with fixes, which you need to review. If you prefer master only, then let me know, and I'll push your patch to master. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2012-01-22 01:29 Message: The patch now only applies to master... should we not try to apply this too cdk-1.4.x? ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2012-01-21 10:39 Message: Added a new patch that addresses all comments - ignore the initial patch. I can't seem to remove the first patch ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2012-01-21 02:59 Message: Rajarshi, my comments... 1. can you please add a test for the writer too, showing that something is fixed, like that crash? 2. Assert.assertTrue(molecule.getAtom(4).getSymbol().equals("Gln")); should be: Assert.assertEquals("Gln", molecule.getAtom(4).getSymbol()); there is a second assertTrue that needs a similar replacement. 3. add the same unit test for the MDLV3000Reader too, please Egon ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3474944&group_id=20024 |