#244 Code clean up in the DeduceBondSystem tool

Needs_Review
closed
nobody
master (162)
5
2012-10-28
2010-08-05
No

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).

Discussion

  • gilleain maclean torrance

    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)

     
  • gilleain maclean torrance

    The current algorithm seems to be:
    1) For each ring, generate a list of positions for possible double bonds.
    2) Generate a set of molecules based on combinations of these positions.
    3) Filter and select the 'best' solution based on the number of 'bad' N/S atoms.
    So, for the failing test, the only way it could pass is for all other solutions to be removed, or it to be ranked as best. Since no solution for this example has N or S atoms, there is no difference at the ranking step.

     
  • Rajarshi Guha

    Rajarshi Guha - 2010-10-16

    Looks OK. Applied and pushed

     

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks