[Cdk-devel] [ cdk-Patches-3040138 ] Code clean up in the DeduceBondSystem tool

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

Patches item #3040138, was opened at 2010-08-05 18:32
Message generated for change (Comment added) made by egonw
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3040138&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 Review
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Egon Willighagen (egonw)
Assigned to: Nobody/Anonymous (nobody)
Summary: Code clean up in the DeduceBondSystem tool

Initial Comment:
Four patches that clean up the code in the DeduceBondSystemTool:

0001 is a request from Nina to be able to set a custom AllRingFinder instance, e.g. with a time-out
0002 is a performance fix... the AllRingFinder has the option for a while now to not find rings larger
      than some threshold. Now, the DeduceBondSystemTool does not deal with rings with more than
      seven atoms anyway, so there's our threshold... should speed up the performance on molecules
      with large ring systems
0003 missing unit tests, as reported by our coverage testing
0004 more QA fixes: missing @TestMethod annotation, JavaDoc, etc.

Note that the last patch also sneaks in a fix that when setInterrupted(true) is called and performed,
that the boolean is reset, to allow further use of the class (as the use case in the unit test class does).

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

>Comment By: Egon Willighagen (egonw)
Date: 2010-08-14 15:12

Message:
Yeah, that's an existing failing unit test:

http://pele.farmbio.uu.se/nightly/test/result-smiles.html

It is indeed not fixed by the code cleanup.

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

Comment By: gilleain maclean torrance (gilleain)
Date: 2010-08-14 15:04

Message:
Looks alright. I guess the main change is the new line:

IRingSet rs = allRingsFinder.findAllRings(m,7);

however, the test xtestQuinone fails with :

java.lang.AssertionError: expected:<SINGLE> but was:<DOUBLE>
org.openscience.cdk.smiles.DeduceBondSystemToolTest.xtestQuinone(DeduceBondSystemToolTest.java:164)

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

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