#76 Patch to solve bug #2855850: setStereo(IBond.Stereo)

Accepted
closed
None
5
2012-10-28
2009-09-10
No

Aimed as CDK master, so 1 senior review required, all other welcome.

The patch changes the CDK API for IBond.set/getStereo() to use IBond.Stereo (new) instead of int. Note that:

  • there are minor code interpretations in SmilesGenerator, ModelBuilder3D and BondTools
  • added two unit tests for MDL molfile reading to test the stereo type mapping

Discussion

  • Rajarshi Guha

    Rajarshi Guha - 2009-09-12

    Looks OK (though it'd be easier if the core changes were in a separate patch).

     
  • Egon Willighagen

    Rajarshi, I am not entirely what you want me to do. Further splitting up the patch is not really possible, as all code needs to be updated for the API changes... all code should be compilable after each commit. I do understand the patch is very long now, but except the API change, there is no new functionality introduced, except for the two unit tests... do I understand correctly you like me to split those out into a separate patch? Or would you like me to do something else?

     
  • Rajarshi Guha

    Rajarshi Guha - 2009-09-12

    but except the API change, there is
    no new functionality introduced, except for the two unit tests... do I
    understand correctly you like me to split those out into a separate patch?

    Yes, that's what I originally meant - but it's not a big deal as the core API change is simple. It was late at night, hence my note :)

    On a related note, I did see one thing this morning. At some points in the patch, the original code had

    if (Math.abs(bond.getStereo()) < 2)

    This was checking for 4 constants - STEREO_DOWN, STEREO_UP, STEREO_DOWN_INV and STEREO_UP_INV

    I'm not sure what the _INV constants indicate compared to the others. But the new code that you add simply checks whether the value is IBond.Stereo.UP or IBond.Stereo.DOWN - do these enums encompass the _INV constants in the original code? If not, then the check is missing 2 conditions

     
  • Egon Willighagen

    Yeah, that bit of code is what made me blogging :)

    The condition actually is >0 and <2... so, considering Math.abs, the options are only -1 and 1... which are UP and DOWN. (I did have to look twice at this code before I understood why the abs was used...)

    The INV, just for the record, is also UP and DOWN, but the order of the atoms is inverted... with _INV, the second atom is stereo center, and not the first, with the regular UP and DOWN...

     
  • Rajarshi Guha

    Rajarshi Guha - 2009-09-12

    Aah, OK. Looks good then

     
  • Rajarshi Guha

    Rajarshi Guha - 2009-11-01

    Applied patch to master and pushed to github

     

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

Sign up for the SourceForge newsletter:





No, thanks