From: SourceForge.net <no...@so...> - 2010-06-20 21:55:09
|
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 |