[ cdk-Patches-2855925 ] Patch to solve bug #2855850: setStereo(IBond.Stereo)

[SourceForge.net]

Patches item #2855925, was opened at 2009-09-10 06:54
Message generated for change (Settings changed) made by rajarshi
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2855925&group_id=20024

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: Needs Review
>Status: Closed
>Resolution: Fixed
Priority: 5
Private: No
Submitted By: Egon Willighagen (egonw)
Assigned to: Rajarshi Guha (rajarshi)
Summary: Patch to solve bug #2855850: setStereo(IBond.Stereo)

Initial Comment:
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

----------------------------------------------------------------------

>Comment By: Rajarshi Guha (rajarshi)
Date: 2009-11-01 09:34

Message:
Applied patch to master and pushed to github

----------------------------------------------------------------------

Comment By: Rajarshi Guha (rajarshi)
Date: 2009-09-12 12:34

Message:
Aah, OK. Looks good then

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2009-09-12 12:32

Message:
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...

----------------------------------------------------------------------

Comment By: Rajarshi Guha (rajarshi)
Date: 2009-09-12 12:17

Message:
> 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

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2009-09-12 12:11

Message:
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?

----------------------------------------------------------------------

Comment By: Rajarshi Guha (rajarshi)
Date: 2009-09-12 00:25

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


----------------------------------------------------------------------

You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2855925&group_id=20024

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
_______________________________________________
Cdk-devel mailing list
Cdk-devel@...
https://lists.sourceforge.net/lists/listinfo/cdk-devel