From: SourceForge.net <no...@so...> - 2009-04-30 15:21:19
|
Patches item #2723203, was opened at 2009-03-31 12:43 Message generated for change (Comment added) made by mark_rynbeek 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 Revision Status: Open Resolution: None Priority: 5 Private: No Submitted By: Mark Rijnbeek (mark_rynbeek) >Assigned to: Egon Willighagen (egonw) 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: Mark Rijnbeek (mark_rynbeek) Date: 2009-04-30 16:21 Message: See new patch attached ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-04-27 12:20 Message: I would very much appreciate if you would make an updated patch. ---------------------------------------------------------------------- Comment By: Mark Rijnbeek (mark_rynbeek) Date: 2009-04-27 09:43 Message: hi Egon, whitespaces duely noted, removal of unused code as well. Regarding the 3e2f71126af5a679dd2670ec45ac495d69b48ae1, I think I must have committed twice in git and this is visible in the patch file as such. Variable maxPathLen does not exist in current version of the AllRingsFinder. Finally the authoring bit was not on purpose. Let me know if you can proceed with the patch, or if you want me to create a new one with your comments taken into account. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-04-24 16:18 Message: Hi Mark, consensus is clearly against my reviewer comment, and to go for overloading... I had a look at your patch. Next time, please keep whitespace changes in a separate patch, as it is much more difficult to see now, what it code change, and what is not (e.g. in 3e2f71126af5a679dd2670ec45ac495d69b48ae1). Otherwise, please remove unused private code instead of commenting it out. Also in 3e2f71126af5a679dd2670ec45ac495d69b48ae1, you have - if (maxPathLen == null || union.size() < maxPathLen) { + if (maxPathLen == null || union.size() <= (maxPathLen+1)) { while the commit message says "breakout on AllRingsFinder"... the code suggests that the break out is already there... or is this unrelated to that in a fix for something else? The third patch, 851e44cc2606bb5b8829d785a5b9e8527d9c45a5, should be removed (I could do that), but one comment on it anyway: - * Copyright (C) 2002-2007 Christoph Steinbeck <ste...@us...> + * Copyright (C) 2002-2009 Christoph Steinbeck <ste...@us...> Since it is you who contributed now, and not Chris, you should keep this line unchanged: * Copyright (C) 2002-2007 Christoph Steinbeck <ste...@us...> and add just below it: * 2009 Mark Rijnbeek <mar...@us...> ---------------------------------------------------------------------- Comment By: Miguel Rojas Cherto (miguelrojasch) Date: 2009-04-23 14:38 Message: About the discussion what is better for the two classes (AllRingsFinder + MaxSizedRingFinder) or overloading and adding a parameter)? In my opinion depends of the user working with the method, better saying depending of his/her experience. A) Overloading: It looks cleaner and structured when you have overloading BUT its makes complicate because you need to read the meaning of the parameters and what is the default value when you don't set one. I find similar problematic when one of my colics wrote a java.util.List object using the add method. B) Two classes: It is easier for new people using the specific class. You have with the name specific functionality. Well my point of view is that I would like to have overloading methods. Seems more professional but more complicate, true. Otherwise let see some case. If we think that it could exist 10 different functions constraining the search of rings means that we should have 10 methods with different names. I think it is easier in this case a overloading method incorporating in one all parameters. Miquel ---------------------------------------------------------------------- Comment By: Christoph Steinbeck (steinbeck) Date: 2009-04-01 11:31 Message: I would definitely vote for the overloading. Besides the fact that "AllRingsForASetMaxSizeFinder" wins the absurdity contest for this month :-), I agree with Rajarhi that the "new functionality" is close enough to the finding all rings. >From the perspective of a person not too familiar with the CDK, looking for this or that functionality, I would find it much more convenient to look at the API doc of only one obvious candidate, in this case the AllRingsFinder and discover there, that, hey, I can actually restrict the ring size, cool. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-04-01 05:48 Message: Then we need a third reviewer :) Miguel, what do you think is best in line with the CDK API philosophy? Two classes (AllRingsFinder + MaxSizedRingFinder) or overloading and adding a parameter)? ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2009-03-31 19:40 Message: As Mark noted, overloading or a new class is taste :) My taste is for overloading (at least a user goes to a single class to do stuff related to ring finding) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-03-31 19:34 Message: Oh, and thanx for putting up the git! ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-03-31 19:32 Message: Mark, that clone you did, checkout out both branches for you. You can use 'git status' and 'git branch' to see which branch you are in, but the default is 'master'. I can see from your git repository from which one you branched. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-03-31 19: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 19: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 17: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 17: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 17: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 14: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 |