#587 SDFReader / Writer support for radicals

Accepted
closed
nobody
None
master
1
2013-03-26
2012-11-26
SBeisken
No

The SDFWriter (MDLV2000Writer) doesn't support writing radicals. I have added support for that and corrected the SDFReader (MDLV2000Reader) to interpret the singlet, doublet, and triplet values correctly according to the V2000 format (singlet, value 2, electrons 1; doublet, value 1, electrons 2; triplet, value 3, electrons 3). [patch attached]

1 Attachments

Discussion

1 2 > >> (Page 1 of 2)
  • John May
    John May
    2012-11-29

    Excellent! No longer will radicals disappear on round tripping :). You could have also added an internal map the enumeration but the switch works okay.

    master-accepted

     
  • John May
    John May
    2012-11-29

    • milestone: Needs_Review --> Accepted
     
    • milestone: Accepted --> Needs_Revision
     
  • Cool! My reviews:

    • make sure to use assertFoo() properly: use the assertEquals(expected, found) pattern (or ask John to blog about assertThat() and use that)

    • I rather have you use a for loop than recursion in writeRadicalPattern() (suggestion)

    • SPIN_MULTIPLICITY.TRIPLET_SPIN sounds a bit duplicate with SPIN twice

    • can you clarify what "NN8" means?

    • please document the public methods of SPIN_MULTIPLICITY (getValue() and getSingleElectrons())

    • you could use a for-loop in testMultipleRadicals()

     
  • SBeisken
    SBeisken
    2012-12-03

    Okay, I have changed the tests to "assertEquals", renamed the enums, added the required comments and documentation. Additional patch attached.

     
    • OK, thanx. I'm on it.

       
  • Stephan, you fixed the wrong asserts... I am sorry I was not clearer... it's the ones in the MDLV2000ReaderTest... like:

    Assert.assertTrue(molecule.getConnectedSingleElectronsCount(molecule.getAtom(0)) == 1);

    There you can use assertEquals(), like:

    Assert.assertEquals(1, molecule.getConnectedSingleElectronsCount(molecule.getAtom(0)));

    The contains for the MDLV2000WriterTest is in fact beter, as the substring indices do not work for me...it's some 4-5 chars of... I guess my molfile output looks slightly different (which is interesting in itself! I suspect the date in the header to be the culprit...)

    Can you please revert to contains() for the writer test, and update the other asserts?

    Please find attached one of the two rebased patches... please continue from those, as the patches did not apply cleanly because of a patch by John...

     
  • John May
    John May
    2012-12-13

    Okay I've had a look but am not sure what the state of this patch is. I also get the substring error.

    Egon - Stephan is on holiday ATM, shall I just update the asserts and you can check it?

     
  • Yes! Unlike paper writing, peer review here does not require the original author to reply/apply comments :)

     
1 2 > >> (Page 1 of 2)