Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#563 Resonance regressions due to IBond.Order

master
closed
None
5
2012-10-08
2007-11-24
Egon Willighagen
No

I have been working on a patch the change the API of IBond, and to change the IBond method:

public Double getOrder()

into

public IBond.Order getOrder()

The result is available from cdk/branches/egonw/ibondorder

but contains a few regressions, particularly in the cdk.reaction module:

Summary:
Fixed:
2
New fails:
13
+Testcase: testResonancePositiveCharge_1(org.openscience.cdk.test.qsar.descriptors.bond.ResonancePositiveChargeDescriptorTest)
+Testcase: testResonancePositiveCharge_2(org.openscience.cdk.test.qsar.descriptors.bond.ResonancePositiveChargeDescriptorTest)
+Testcase: testResonancePositiveCharge_3(org.openscience.cdk.test.qsar.descriptors.bond.ResonancePositiveChargeDescriptorTest)
+Testcase: testResonancePositiveCharge_4(org.openscience.cdk.test.qsar.descriptors.bond.ResonancePositiveChargeDescriptorTest)
+Testcase: testResonancePositiveCharge_5(org.openscience.cdk.test.qsar.descriptors.bond.ResonancePositiveChargeDescriptorTest)
+Testcase: testResonancePositiveCharge_6(org.openscience.cdk.test.qsar.descriptors.bond.ResonancePositiveChargeDescriptorTest)
+Testcase: testResonancePositiveCharge_7(org.openscience.cdk.test.qsar.descriptors.bond.ResonancePositiveChargeDescriptorTest)
+Testcase: testBB_1(org.openscience.cdk.test.reaction.type.BreakingBondReactionTest)
+Testcase: testAutomaticSearchCentreActiveExample1(org.openscience.cdk.test.reaction.type.RearrangementRadical2ReactionTest)
+Testcase: testManuallyPutCentreActiveExample1(org.openscience.cdk.test.reaction.type.RearrangementRadical2ReactionTest)
+Testcase: testAutomaticSearchCentreActiveExample1(org.openscience.cdk.test.reaction.type.RearrangementRadical3ReactionTest)
+Testcase: testManuallyPutCentreActiveExample1(org.openscience.cdk.test.reaction.type.RearrangementRadical3ReactionTest)
+Testcase: testSmiles9(org.openscience.cdk.test.smiles.SmilesParserTest)

The testSmiles9 is a time out, but works when run from within Eclipse. I guess the tests still leak a lot of memory.

I will consult with Miguel to get these regressions fixed, as there is no principle test failing, indicating that our lower level IBond unit tests are not complete.

Additionally, I'll started branch to have the LonePairChecker utility the reaction module relies on, based on the new CDK atom types. Again, together with Miguel.

The branches/egonw/ibondorder/ branch is open for suggestions. You can add FIXMEs and TODOs, by adding comment lines like (but please do not commit anything else than such lines, most certainly not source code style changes):

// FIXME: ibondorder: your comment on what I should fix
// TODO: ibondorder: your comment on what I might add

The extra "ibondorder:" provide me a filter so that I can quickly see in the Eclipse Tasks view what should clean up.

Thanx.

Discussion

  • Rajarshi Guha
    Rajarshi Guha
    2007-12-07

    Logged In: YES
    user_id=349408
    Originator: NO

    Looks good. A few comments
    1. Is there a reason for the enum being in IBond rather than CDKConstants? The bond order values are constants

    1. Why is there no value for aromatic? I realize that aromaticity will be indicated by the AROMATIC flag on a bond, so maybe it's moot
     
  • Logged In: YES
    user_id=25678
    Originator: YES

    Ad1: Unlike the constants in CDKConstants, the enumeration also defines a new Class 'IBond.Order', which is used in the interfaces.

    Ad2: Correct. The flag takes or AROMATIC. IBond.Order is a direct translation of the use of double, where we, long time ago, phased out the 1.5 double for aromatic bonds.

     
  • Logged In: YES
    user_id=25678
    Originator: YES

    Patch applied to trunk/. Moving the report to bugs, because the new failing unit tests.

     
  • Logged In: YES
    user_id=25678
    Originator: YES

    This is being addressed in branches/miguelrojasch/reaction/.

     
  • Logged In: YES
    user_id=25678
    Originator: YES

    Miguel fixed these issues.