Help save net neutrality! Learn more.
Close

#587 SDFReader / Writer support for radicals

Accepted
closed
nobody
None
master
1
2013-03-26
2012-11-26
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

  • 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
     
  • Egon Willighagen

    • milestone: Accepted --> Needs_Revision
     
  • Egon Willighagen

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

     
  • Anonymous - 2012-12-03

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

     
    • Egon Willighagen

      OK, thanx. I'm on it.

       
  • Egon Willighagen

    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?

     
  • Egon Willighagen

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

     
  • John May

    John May - 2012-12-13

    Okay, I've changed all to assertThat and replaced the substring with an exact string comparison on the line at the expected index.

    Both commits are in this file.

    • also, we're down to only 6 fails in master/io
     
  • Egon Willighagen

    Cool, I did not it was possible to concatenate patches like this, and still be able to just do 'git am' :)

     
  • John May

    John May - 2013-03-26
    • status: open --> closed
    • milestone: Needs_Revision --> Accepted
     
  • John May

    John May - 2013-03-26

    this has been applied and pushed by Egon

     

Log in to post a comment.