[Cdk-devel] [ cdk-Patches-3017759 ] Signature functionality

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

Patches item #3017759, was opened at 2010-06-17 17:06
Message generated for change (Comment added) made by rajarshi
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3017759&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: master
Group: Needs Revision
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Egon Willighagen (egonw)
Assigned to: Rajarshi Guha (rajarshi)
Summary: Signature functionality

Initial Comment:
Patch that introduced signature calculation for the CDK. The patch is developed by Gilliean, with some patched here and there from me to address CDK stability issues. Aimed at CDK master. The patch is mostly wrapping around the code from in the signature.jar from Gilleain's signature project:

http://github.com/gilleain/signatures

With some discussion here:

http://gilleain.blogspot.com/2009/06/more-chemical-signature-example.html

Reviewing most welcome. The patch is available from the 155-signatures branch at my account:

http://github.com/egonw/cdk/tree/155-signatures

You need commit  d01f1e8bab6c91daa6e9.

----------------------------------------------------------------------

>Comment By: Rajarshi Guha (rajarshi)
Date: 2010-06-20 17:55

Message:
I pulled the latest code (using Egon repo as the remote) and I don't see
the AtomSignature constructor that takes an IAtom.

Regarding docs for the AbstractGraphSignature - would the @inherits
Javadoc tag be of use here?

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2010-06-20 16:34

Message:
Pushed to my 155-signatures branch, and added two commits extra:

1. updated src/META-INF/*signatures.libdepends
2. added missing jar/signatures-1.0.jar.meta

Rajarshi, are your questions sufficiently answered?

----------------------------------------------------------------------

Comment By: gilleain maclean torrance (gilleain)
Date: 2010-06-20 12:30

Message:
Changes committed and pushed to the 155-signatures branch on my github-cdk.

----------------------------------------------------------------------

Comment By: gilleain maclean torrance (gilleain)
Date: 2010-06-19 17:47

Message:
1) The signature implementation tries to be toolkit-independant, and is a
separate project.
2) Agreed. Will add.
3) It doesn't, and is a good idea not to show it, as it is only intended
to be used by the base class.
4) Yes, it is definitely useful. It's a bit of a nasty design that an
'Orbit' is essentially the same as a 'SymmetryClass' as both are just a
list of atom (indices) and a string label. Roughly speaking, all atoms in
the same orbit are equivalent - they have the same signature.

One annoying aspect of the design is that there are useful methods in the
abstract base classes - especially AbstractGraphSignature - that may not be
clear to CDK users. I don't know how the javadocs or other documentation
for the signature library code can be smoothly included in the CDK.

----------------------------------------------------------------------

Comment By: Rajarshi Guha (rajarshi)
Date: 2010-06-19 17:26

Message:
Looks good. A few issues.

1) Is there a reason for the signature code to be in a separate jar file?
What is in that file (the signature-SNAPSHOT)?

2) For AtomSignature, it'd be useful to have a constructor that took an
IAtom as a first arhument

3) Does getConnected() In AtomSignature have to be public? If so, this is
a rather general function and could be made prt of IAtomContainer. I'd
rather see it private or protected

4) In MolecularSignature is it useful to have getOrbits as public? Is an
Orbit object useful on its own? Could it be made protected? Similarly for
the Orbit class itself. Can that be made protected?

(My aim in the last two points is to keep the API simple and avoid
extraneous mehtods/classes

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2010-06-17 17:10

Message:
Reviewing of the API is most important, but any comment is, as always,
welcome.

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2010-06-17 17:09

Message:
The Nightly reports are here:

http://pele.farmbio.uu.se/nightly-signatures/

All is clean (fully unit tests, OK JavaDoc, mostly OK on PMD), except for
some false positives from PDM.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3017759&group_id=20024