Menu
▾
▴
[Cdk-devel] [ cdk-Patches-3395661 ] Se Atom type added
|
From: SourceForge.net <no...@so...> - 2011-08-29 10:18:42
|
Patches item #3395661, was opened at 2011-08-21 13:08 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3395661&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: Open Resolution: None Priority: 5 Private: No Submitted By: Asad (asadrahman) Assigned to: Asad (asadrahman) Summary: Se Atom type added Initial Comment: This is a reviewed atom time from KEGG atom types. Now all the test cases behave and in one of the test cases I have made Se.3 to Se.2 as it was a wrong test case. ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2011-08-29 12:18 Message: Asad, this is cheminformatics to you... you cannot assume a change does not break anything. It commonly does :) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-08-29 12:18 Message: No, I would not say so. The labels actually have no meaning, and the .2 actually referred to the hybridization state. So, your assumption was actually not valid. But, importantly, it changes behavior without actually fixing anything. That should be prevented at all cost. Atom type IDs are use as-is, and changing it breaks things, like atom-type-2-atom-type maps. In fact, while there is no unit test for that at this moment, it actually broken Sybyl atom type perception. ---------------------------------------------------------------------- Comment By: Asad (asadrahman) Date: 2011-08-29 11:08 Message: Se.3 was changed to Se.2 as it checks for 2 neighbours not 3 in the test case. Hence I assume this change is a valid one! ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-08-28 21:12 Message: This patch changes the definition of an existing atom type. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3395661&group_id=20024 |