[Cdk-devel] [ cdk-Patches-2740730 ] SSSRFinder fix

[SourceForge.net <no...@so...>]

Patches item #2740730, was opened at 2009-04-07 15:34
Message generated for change (Comment added) made by egonw
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2740730&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: Accepted
>Status: Closed
>Resolution: Fixed
Priority: 5
Private: No
Submitted By: Mark Rijnbeek (mark_rynbeek)
Assigned to: Egon Willighagen (egonw)
Summary: SSSRFinder fix

Initial Comment:

Some issues with SSSRFinder have come to light, Ulrich Bauer has been contacted and is having a look.

I've created a new patch to make the following fixes:
- correct implementation of methods findEssentialRings(), findRelevantRings() and findSSSR() in SSSRFinder
- avoiding duplicate ringsets in output result (occurs with bucky balls sometimes, example handed over to Ulrich)

Initially: no files/patch attached, but see e-mail below for more info

_____________________________

------- Original Message --------
Subject: 	Re: SSSR (CDK)
Date: 	Mon, 6 Apr 2009 15:28:12 +0200
From: 	Ulrich Bauer <ba...@ma...>
To: 	Mark Rijnbeek <ma...@eb...>

>> Hi Mark,
>>
>> this error seems to have crept in at revisions 5704 and 12004. Now  
>> cycleBasis().cycles() is returned instead of  
>> cycleBasis().relevantCycles().keySet(); similarly for the essential  
>> cycles. Maybe you can fix this, I do not have my repository set up  
>> and working yet.
>>
>> Best regards, Ulrich
>>
>

> I noticed the difference for the methods, one returns a Map, the  
> other a Collection.
> So the change would be as below ?
>
>   public IRingSet findEssentialRings() {
>       if (atomContainer==null) {
>           return null;
>       }
>       IRingSet ringSet =  
> toRingSet(atomContainer,cycleBasis().essentialCycles());
>       return ringSet;
>   }
>
>   public IRingSet findRelevantRings() {
>       if (atomContainer==null) {
>           return null;
>       }
>             IRingSet ringSet = toRingSet(atomContainer,  
> cycleBasis().relevantCycles().keySet());
>       return ringSet;        }
>
>
> I'll ask for a patch on cdk-devel.
>

Yes, that change looks correct. Thanks for the info about git.
The Map returned by cycleBasis().relevantCycles() provides additional  
information about the relevant cycles; since we are only interested in  
the cycles themselves, the keySet() of this map is needed.

Ulrich
-----------------------------



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

>Comment By: Egon Willighagen (egonw)
Date: 2009-12-23 13:44

Message:
Applied to master.

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

Comment By: Mark Rijnbeek (mark_rynbeek)
Date: 2009-10-26 18:05

Message:
>>>The patch has a few shortcomings... 
Which are?

>>but most importantly, it lack unit tests...
Done, new attachment

>>For the rest, I'd like to see it use parameterized types: List<Foo>
instead of just List, etc.
I tried for example 
List<Edge> edges = edgesOf(vertex);
.. but then get IDE warnings (unsound conversion), and that's because in
org._3pq
public java.util.List edgesOf(java.lang.Object p1) is not parameterized.
So I won't make changes there.



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

Comment By: Egon Willighagen (egonw)
Date: 2009-09-18 22:53

Message:
The patch has a few shortcomings... but most importantly, it lack unit
tests...

For the rest, I'd like to see it use parameterized types: List<Foo>
instead of just List, etc.

BTW, anyone can fix such shortcomings... 

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

Comment By: Mark Rijnbeek (mark_rynbeek)
Date: 2009-07-27 17:59

Message:
Patch received from Ulrich Bauer, see attachment - git patch

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

Comment By: Mark Rijnbeek (mark_rynbeek)
Date: 2009-06-10 10:28

Message:

More fixes should be included in this patch than just the one mentioned in
the report. But Egon (7th april) suggested "Please file that quick fix
anyway, so that we have that as back up when the other fix takes more
time..." 
Therefore the empty patch; now waiting for fixes from Ulrich (none so far
) to be combined into one patch to fix bugs in the SSSRFinder.

I can make a  small patch just fixing what is listed in the report text.
But I am actually not sure that those suggested lines work. I gave a bucky
ball to the SSSRFinder with these new code lines, but got a whole new error
thrown.

If Ulrich doesn't have time for a fix of the SSSRFinder then someone else
will have to do it. I'll check with Chris if we should do it here at the
EBI. It's not trivial, the bigger patch would need to fix the current
duplicate ring sets that can occur in the output of SSSRFinder when
processing a bucky ball.




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

Comment By: Rajarshi Guha (rajarshi)
Date: 2009-06-10 04:11

Message:
I see the changes in the text of the report and they look OK - but it would
be useful to have a patch and associated unit tests (if relevant) that I
could apply to a branch

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

Comment By: Stefan Kuhn (shk3)
Date: 2009-06-09 17:26

Message:
I can't really see what's the patch here.

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

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