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.
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Here .2 doesn't represents the hybridization state but the neighbour
count. The hybridization state is represented by .sp2, .sp3 etc."
Huh? How do you know that the person who added the existing Se atom type meant that? These labels do not have any semantics, they're just identifiers, and identifiers should be stable. As far as I can see, there is no existing bug. And, for information, the use of .2 was inherited from Sybyl atom types, which use that to indicate hybridization:
Ok, so the newly added patches now should not touch the "Se.3" atomtype, but add a new "Se.sp3.3" type (following the same naming system as other added types).
Based from af9542aa53cbb1e0957b7ab822ce8186bcbe6917
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
This patch changes the definition of an existing atom type.
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!
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.
Asad, this is cheminformatics to you... you cannot assume a change does not break anything. It commonly does :)
Well I think you are mixing two things...
Here .2 doesn't represents the hybridization state but the neighbour count. The hybridization state is represented by .sp2, .sp3 etc.
For example Se.3 and Se.2 have same hybridaztion state but different neighbor count.
<at:AtomType rdf:ID="Se.2">
<at:formalCharge>0</at:formalCharge>
<at:hasElement rdf:resource="&elem;Se"/>
<at:formalNeighbourCount>2</at:formalNeighbourCount>
<at:lonePairCount>2</at:lonePairCount>
<at:piBondCount>0</at:piBondCount>
<at:hybridization rdf:resource="&at;sp3"/>
</at:AtomType>
Hence either that test case is wrong or we need to change the label in the test case :-)
Yes you are right it might break things but actually fixing this will eventually fix all the wrong cases too....
"Well I think you are mixing two things...
Here .2 doesn't represents the hybridization state but the neighbour
count. The hybridization state is represented by .sp2, .sp3 etc."
Huh? How do you know that the person who added the existing Se atom type meant that? These labels do not have any semantics, they're just identifiers, and identifiers should be stable. As far as I can see, there is no existing bug. And, for information, the use of .2 was inherited from Sybyl atom types, which use that to indicate hybridization:
http://tripos.com/mol2/atom_types.html
BTW, how to you get a pi bond with sp3 hybridization, which does not have a free p orbital that can form a pi bond?
new version of path as of 08/09/11
new version of patch as of 08/09/11
new version of patch as of 08/09/11
Ok, so the newly added patches now should not touch the "Se.3" atomtype, but add a new "Se.sp3.3" type (following the same naming system as other added types).
Based from af9542aa53cbb1e0957b7ab822ce8186bcbe6917
Thanx for fixing that. I have looked it, and it looks OK, so I applied it to cdk-1.4.x, fixing quite a few missing atom types.