From: SourceForge.net <no...@so...> - 2009-03-31 18:30:13
|
Patches item #2723203, was opened at 2009-03-31 13:43 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2723203&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: None Group: Needs Review Status: Open Resolution: None Priority: 5 Private: No Submitted By: Mark Rijnbeek (mark_rynbeek) Assigned to: Nobody/Anonymous (nobody) Summary: AllRingsFinder recursion break out Initial Comment: For fingerprinting (OrChem) I need to find rings in big compounds such as bucky balls. But these compounds cause a time out, or do not return in acceptable time. However, my fingerprinter does not care about rings over a certain size. This patch allows me to find and count all rings in for example bucky balls by limiting the search depth as I require, looking for rings up til for example size six. AllRingsFinder: - findAllRings() and findAllRingsInIsolatedRingSystem() have been overloaded - the overloading is such that the change has no effect on current invokers - the overloading allows to find all rings up to a certain max size - the max size is inspected at two added if-statements in method remove() - I commented out method removeAliphatic() - it was private and not used in the class itself (highlighted by IDE) AllRingsFinderTest - added 3 new test cases to test overloading and max depth searching on big compounds ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2009-03-31 20:30 Message: Rajarshi, I prefer to have larger algorithms in isolated classes, as to make there differences clear, as to add parameters cluters the API. And from a naming perspective, all rings is no longer all rings, if only a subset if used. Then it should be named AllRingsForASetMaxSizeFinder. IMO ---------------------------------------------------------------------- Comment By: Mark Rijnbeek (mark_rynbeek) Date: 2009-03-31 20:25 Message: Regarding trunk or 1.2.x, all I know is I got the source code from git using git clone git://cdk.git.sourceforge.net/gitroot/cdk After that, I made a branch and worked on that. I never typed 'trunk' or '1.2.x' at any point , as far as I can remember. Furthermore could we agree on whether to overload (first solution) or to subclass (suggested by Egon)? Both fine by me, it's really a matter of taste. thanks M ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2009-03-31 18:38 Message: One comment on the patch 1. If the method is not used, I'd just remove it rather than comment it out ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2009-03-31 18:36 Message: I would actually like to keep it as addtions to AllRingsFinder - 1. I'd prefer not to enlarge the API with closely related classes 2. One could say that Marks patch identies "all rings" upto a given size, so is semantically close enough to the original function Regarding patch against trunk or 1.2.x - which code base did you work of? ---------------------------------------------------------------------- Comment By: Mark Rijnbeek (mark_rynbeek) Date: 2009-03-31 18:16 Message: We don't have a git server here that I know of, but according to the doc I don't necessarily need one (http://www.kernel.org/pub/software/scm/git/docs/user-manual.html#setting-up-a-public-repository) I read instructions for "Exporting a git repository via http". You can use URL http://www.ebi.ac.uk/~markr/cdk.git/ I made the changes as suggested, new abstract class RingFinder has the algorithms, class AllRingsFinder and new class MaxSizedRingFinder extend it. Regarding patching trunk or cdk-1.2.x, I actually don't understand how these two relate to each other. Could you explain? Mark ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-03-31 15:00 Message: Mark, can you make this patch available as git branch on an EBI server? Since the purpose of the AllRingsFinder is to find *all* rings, I suggest factoring out your code into a MaxSizedRingFinder, with optionally a third class with shared algorithms. Please also indicate if your patch is for trunk or for cdk-1.2.x. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2723203&group_id=20024 |