[Cdk-devel] [ cdk-Patches-2723203 ] AllRingsFinder recursion break out

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

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: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 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