Administrator
|
Hi there,
To help with getting the release together, I've run the ABI compliance checker on the current dev code versus 2.3.2. The result is available at: http://baoilleach.webfactional.com/site_media/ob_api_check/compat_report_21052016.html It makes a few mistakes here and there but is accurate in general. I have a few comments to make regarding some of the changes, but will do that separately. - Noel ------------------------------------------------------------------------------ Mobile security can be enabling, not merely restricting. Employees who bring their own devices (BYOD) to work are irked by the imposition of MDM restrictions. Mobile Device Manager Plus allows you to control only the apps on BYO-devices by containerizing them, leaving personal data untouched! https://ad.doubleclick.net/ddm/clk/304595813;131938128;j _______________________________________________ OpenBabel-Devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/openbabel-devel |
> http://baoilleach.webfactional.com/site_media/ob_api_check/compat_report_21052016.html
> It makes a few mistakes here and there but is accurate in general. It's a bit weird about adding optional parameters, e.g. bool Build(OBMol &mol); becomes bool Build(OBMol &mol, bool stereoWarnings = true); This isn't an API issue, since a program build to the first will still recompile. This is an ABI problem, but we've never been too concerned about that from 2.x to 2.y. > I have a few comments to make regarding some of the changes, but will do that separately. I'm obviously interested in the feedback. It's also useful to make sure that source documentation was added. Thanks, -Geoff ------------------------------------------------------------------------------ Mobile security can be enabling, not merely restricting. Employees who bring their own devices (BYOD) to work are irked by the imposition of MDM restrictions. Mobile Device Manager Plus allows you to control only the apps on BYO-devices by containerizing them, leaving personal data untouched! https://ad.doubleclick.net/ddm/clk/304595813;131938128;j _______________________________________________ OpenBabel-Devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/openbabel-devel |
Administrator
|
I was just going to make the point that I'm not too keen on the
majority of the additions to OBAtom and OBMol, which appear to be convenience functions. Consider IsSulfoneOxygen. There are a lot of elements out there and a lot of functional groups; we should either add them all or add none. If in the end, this function is absolutely necessary, surely such convenience functions could live elsewhere rather than being in the central classes of the library. I blame IsHydrogen() - it's a magnet for further "Is"s :-) - Noel On 23 May 2016 at 19:06, Geoffrey Hutchison <[hidden email]> wrote: >> http://baoilleach.webfactional.com/site_media/ob_api_check/compat_report_21052016.html >> It makes a few mistakes here and there but is accurate in general. > > It's a bit weird about adding optional parameters, e.g. > bool Build(OBMol &mol); > becomes > bool Build(OBMol &mol, bool stereoWarnings = true); > > This isn't an API issue, since a program build to the first will still recompile. This is an ABI problem, but we've never been too concerned about that from 2.x to 2.y. > >> I have a few comments to make regarding some of the changes, but will do that separately. > > I'm obviously interested in the feedback. It's also useful to make sure that source documentation was added. > > Thanks, > -Geoff ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e _______________________________________________ OpenBabel-Devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/openbabel-devel |
I welcome this discussion because it addresses some of the issues I had when starting
contributing to the code. I'm partially responsible of several convenience functions (IsSulfoneOxygen, to name one :), and I would be happy to have a guideline to follow to deal with the issue. The rationale is: I've been schooled by experienced programmers to keep functions as simple as possible in order to limit bugs (like that rock band in which they paint their faces black and white). Prior to my patch, IsHbondAcceptor was just a handful of lines, and adding features made it grow to several 'pages' of source. Adding convenience functions was an attempt to improve code clarity and reduce chances of bugs. Although, I agree with Noel in terms of not creating ad-hoc functions which are used only once. If there's a way to group them together to keep them isolated from the main code (or not public?), I'm fine with it. I'm a big fan of generalizations over special cases. A similar thing is happening for the OBBond::IsEster function, but in this case I don't know how to deal with new features. I've tried fixing a possible bug and extend the functionalities, so the code went from 29 to 296 lines (including some documentation/comments). In order to cover all esters accordingly to IUPAC (and Wikipedia, of course), I ended up in writing extra functions that are called by IsEster itself, but can still be called independently: IsEster: ->IsPhosphateEster ->IsCarboxyEster ->IsThioEster As a consequence, the ABI would change too, and IsEster now could accept several extra boolean flags to deal with these extra features (before it accepted none). I think these extra functions could be considered convenience functions. In principle, the whole code could be in a single, larger function, which will have one well-defined behavior. Although, I think this would imply the inability to customize the behavior of IsEster, and limit the access to new features. In a possible scenario, a user could call IsEster(), and then write its own code to get discriminate between the different ester types. Since the function already did the job, why not make it available directly? I'm aware that this is a complex reply to what was a simple and well-posed point, but I would appreciate to have such aspects clarified to prevent bad coding practices to sneak in the project. It would be great if eventually such guidelines would land in a nice and small "Guidelines" page for OB developers and contributors. Thanks! S On 05/27/2016 08:24 AM, Noel O'Boyle wrote: > I was just going to make the point that I'm not too keen on the > majority of the additions to OBAtom and OBMol, which appear to be > convenience functions. Consider IsSulfoneOxygen. There are a lot of > elements out there and a lot of functional groups; we should either > add them all or add none. If in the end, this function is absolutely > necessary, surely such convenience functions could live elsewhere > rather than being in the central classes of the library. > > I blame IsHydrogen() - it's a magnet for further "Is"s :-) > > - Noel > > On 23 May 2016 at 19:06, Geoffrey Hutchison <[hidden email]> wrote: >>> http://baoilleach.webfactional.com/site_media/ob_api_check/compat_report_21052016.html >>> It makes a few mistakes here and there but is accurate in general. >> >> It's a bit weird about adding optional parameters, e.g. >> bool Build(OBMol &mol); >> becomes >> bool Build(OBMol &mol, bool stereoWarnings = true); >> >> This isn't an API issue, since a program build to the first will still recompile. This is an ABI problem, but we've never been too concerned about that from 2.x to 2.y. >> >>> I have a few comments to make regarding some of the changes, but will do that separately. >> >> I'm obviously interested in the feedback. It's also useful to make sure that source documentation was added. >> >> Thanks, >> -Geoff > > ------------------------------------------------------------------------------ > What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic > patterns at an interface-level. Reveals which users, apps, and protocols are > consuming the most bandwidth. Provides multi-vendor support for NetFlow, > J-Flow, sFlow and other flows. Make informed decisions using capacity > planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e > _______________________________________________ > OpenBabel-Devel mailing list > [hidden email] > https://lists.sourceforge.net/lists/listinfo/openbabel-devel > -- Stefano Forli, PhD Assistant Professor of Integrative Structural and Computational Biology, Molecular Graphics Laboratory Dept. of Integrative Structural and Computational Biology, MB-112A The Scripps Research Institute 10550 North Torrey Pines Road La Jolla, CA 92037-1000, USA. tel: +1 (858)784-2055 fax: +1 (858)784-2860 email: [hidden email] http://www.scripps.edu/~forli/ ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e _______________________________________________ OpenBabel-Devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/openbabel-devel |
On 27/05/16 20:47, Stefano Forli wrote:
> I welcome this discussion because it addresses some of the issues I had when starting > contributing to the code. > > I'm partially responsible of several convenience functions (IsSulfoneOxygen, to name one > :), and I would be happy to have a guideline to follow to deal with the issue. > > The rationale is: I've been schooled by experienced programmers to keep functions as > simple as possible in order to limit bugs (like that rock band in which they paint their > faces black and white). > > Prior to my patch, IsHbondAcceptor was just a handful of lines, and adding features made > it grow to several 'pages' of source. Adding convenience functions was an attempt to > improve code clarity and reduce chances of bugs. > Although, I agree with Noel in terms of not creating ad-hoc functions which are used only > once. If there's a way to group them together to keep them isolated from the main code (or > not public?), I'm fine with it. > I'm a big fan of generalizations over special cases. > > A similar thing is happening for the OBBond::IsEster function, but in this case I don't > know how to deal with new features. > I've tried fixing a possible bug and extend the functionalities, so the code went from 29 > to 296 lines (including some documentation/comments). > In order to cover all esters accordingly to IUPAC (and Wikipedia, of course), I ended up > in writing extra functions that are called by IsEster itself, but can still be called > independently: > > IsEster: > ->IsPhosphateEster > ->IsCarboxyEster > ->IsThioEster > > As a consequence, the ABI would change too, and IsEster now could accept several extra > boolean flags to deal with these extra features (before it accepted none). > > I think these extra functions could be considered convenience functions. In principle, the > whole code could be in a single, larger function, which will have one well-defined behavior. > > Although, I think this would imply the inability to customize the behavior of IsEster, and > limit the access to new features. > > In a possible scenario, a user could call IsEster(), and then write its own code to get > discriminate between the different ester types. Since the function already did the job, > why not make it available directly? > > I'm aware that this is a complex reply to what was a simple and well-posed point, but I > would appreciate to have such aspects clarified to prevent bad coding practices to sneak > in the project. > > It would be great if eventually such guidelines would land in a nice and small > "Guidelines" page for OB developers and contributors. > once is to have a function called enum FunctionalGroup OBMol::functionalGroup() {} with enum { FunctionalGroup_PhosphateEster, FunctionalGroup_Amine, FunctionalGroup_Alcohol }; In this manner support for new functional groups can be added under the hood without changing the interface. > Thanks! > > S > > > > On 05/27/2016 08:24 AM, Noel O'Boyle wrote: >> I was just going to make the point that I'm not too keen on the >> majority of the additions to OBAtom and OBMol, which appear to be >> convenience functions. Consider IsSulfoneOxygen. There are a lot of >> elements out there and a lot of functional groups; we should either >> add them all or add none. If in the end, this function is absolutely >> necessary, surely such convenience functions could live elsewhere >> rather than being in the central classes of the library. >> >> I blame IsHydrogen() - it's a magnet for further "Is"s :-) >> >> - Noel >> >> On 23 May 2016 at 19:06, Geoffrey Hutchison <[hidden email]> wrote: >>>> http://baoilleach.webfactional.com/site_media/ob_api_check/compat_report_21052016.html >>>> It makes a few mistakes here and there but is accurate in general. >>> >>> It's a bit weird about adding optional parameters, e.g. >>> bool Build(OBMol &mol); >>> becomes >>> bool Build(OBMol &mol, bool stereoWarnings = true); >>> >>> This isn't an API issue, since a program build to the first will still recompile. This is an ABI problem, but we've never been too concerned about that from 2.x to 2.y. >>> >>>> I have a few comments to make regarding some of the changes, but will do that separately. >>> >>> I'm obviously interested in the feedback. It's also useful to make sure that source documentation was added. >>> >>> Thanks, >>> -Geoff >> >> ------------------------------------------------------------------------------ >> What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic >> patterns at an interface-level. Reveals which users, apps, and protocols are >> consuming the most bandwidth. Provides multi-vendor support for NetFlow, >> J-Flow, sFlow and other flows. Make informed decisions using capacity >> planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e >> _______________________________________________ >> OpenBabel-Devel mailing list >> [hidden email] >> https://lists.sourceforge.net/lists/listinfo/openbabel-devel >> > -- David van der Spoel, Ph.D., Professor of Biology Dept. of Cell & Molec. Biol., Uppsala University. Box 596, 75124 Uppsala, Sweden. Phone: +46184714205. [hidden email] http://folding.bmc.uu.se ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e _______________________________________________ OpenBabel-Devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/openbabel-devel |
Administrator
|
In reply to this post by Stefano Forli
Hi Stefano,
Short functions are good - I should really be better at this - but there is no need to add all of your functions to the public API. From what you say, it sounds like this was unintentional. In that case, is it okay if I change things to remove these utility functions from the public API? This will involve moving them from member functions of OBAtom/OBBond to static functions, e.g. from OBAtom::IsXXXX() to static OBAtom_IsXXXX(OBAtom*). Functionally everything should be the same. Regards, - Noel On 27 May 2016 at 19:47, Stefano Forli <[hidden email]> wrote: > I welcome this discussion because it addresses some of the issues I had when > starting contributing to the code. > > I'm partially responsible of several convenience functions (IsSulfoneOxygen, > to name one :), and I would be happy to have a guideline to follow to deal > with the issue. > > The rationale is: I've been schooled by experienced programmers to keep > functions as simple as possible in order to limit bugs (like that rock band > in which they paint their faces black and white). > > Prior to my patch, IsHbondAcceptor was just a handful of lines, and adding > features made it grow to several 'pages' of source. Adding convenience > functions was an attempt to improve code clarity and reduce chances of bugs. > Although, I agree with Noel in terms of not creating ad-hoc functions which > are used only once. If there's a way to group them together to keep them > isolated from the main code (or not public?), I'm fine with it. > I'm a big fan of generalizations over special cases. > > A similar thing is happening for the OBBond::IsEster function, but in this > case I don't know how to deal with new features. > I've tried fixing a possible bug and extend the functionalities, so the code > went from 29 to 296 lines (including some documentation/comments). > In order to cover all esters accordingly to IUPAC (and Wikipedia, of > course), I ended up in writing extra functions that are called by IsEster > itself, but can still be called independently: > > IsEster: > ->IsPhosphateEster > ->IsCarboxyEster > ->IsThioEster > > As a consequence, the ABI would change too, and IsEster now could accept > several extra boolean flags to deal with these extra features (before it > accepted none). > > I think these extra functions could be considered convenience functions. In > principle, the whole code could be in a single, larger function, which will > have one well-defined behavior. > > Although, I think this would imply the inability to customize the behavior > of IsEster, and limit the access to new features. > > In a possible scenario, a user could call IsEster(), and then write its own > code to get discriminate between the different ester types. Since the > function already did the job, why not make it available directly? > > I'm aware that this is a complex reply to what was a simple and well-posed > point, but I would appreciate to have such aspects clarified to prevent bad > coding practices to sneak in the project. > > It would be great if eventually such guidelines would land in a nice and > small "Guidelines" page for OB developers and contributors. > > Thanks! > > S > > > > > On 05/27/2016 08:24 AM, Noel O'Boyle wrote: >> >> I was just going to make the point that I'm not too keen on the >> majority of the additions to OBAtom and OBMol, which appear to be >> convenience functions. Consider IsSulfoneOxygen. There are a lot of >> elements out there and a lot of functional groups; we should either >> add them all or add none. If in the end, this function is absolutely >> necessary, surely such convenience functions could live elsewhere >> rather than being in the central classes of the library. >> >> I blame IsHydrogen() - it's a magnet for further "Is"s :-) >> >> - Noel >> >> On 23 May 2016 at 19:06, Geoffrey Hutchison <[hidden email]> >> wrote: >>>> >>>> >>>> http://baoilleach.webfactional.com/site_media/ob_api_check/compat_report_21052016.html >>>> It makes a few mistakes here and there but is accurate in general. >>> >>> >>> It's a bit weird about adding optional parameters, e.g. >>> bool Build(OBMol &mol); >>> becomes >>> bool Build(OBMol &mol, bool stereoWarnings = true); >>> >>> This isn't an API issue, since a program build to the first will still >>> recompile. This is an ABI problem, but we've never been too concerned about >>> that from 2.x to 2.y. >>> >>>> I have a few comments to make regarding some of the changes, but will do >>>> that separately. >>> >>> >>> I'm obviously interested in the feedback. It's also useful to make sure >>> that source documentation was added. >>> >>> Thanks, >>> -Geoff >> >> >> >> ------------------------------------------------------------------------------ >> What NetFlow Analyzer can do for you? Monitors network bandwidth and >> traffic >> patterns at an interface-level. Reveals which users, apps, and protocols >> are >> consuming the most bandwidth. Provides multi-vendor support for NetFlow, >> J-Flow, sFlow and other flows. Make informed decisions using capacity >> planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e >> _______________________________________________ >> OpenBabel-Devel mailing list >> [hidden email] >> https://lists.sourceforge.net/lists/listinfo/openbabel-devel >> > > -- > > Stefano Forli, PhD > > Assistant Professor of Integrative > Structural and Computational Biology, > Molecular Graphics Laboratory > > Dept. of Integrative Structural > and Computational Biology, MB-112A > The Scripps Research Institute > 10550 North Torrey Pines Road > La Jolla, CA 92037-1000, USA. > > tel: +1 (858)784-2055 > fax: +1 (858)784-2860 > email: [hidden email] > http://www.scripps.edu/~forli/ ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e _______________________________________________ OpenBabel-Devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/openbabel-devel |
Administrator
|
Sorry - I've just reread your reply again. In the case of the OBBond,
it sounds like you think it would be useful for these to be in the public API. Maybe I should look through these functions a bit first. - Noel On 2 June 2016 at 07:33, Noel O'Boyle <[hidden email]> wrote: > Hi Stefano, > > Short functions are good - I should really be better at this - but > there is no need to add all of your functions to the public API. From > what you say, it sounds like this was unintentional. In that case, is > it okay if I change things to remove these utility functions from the > public API? This will involve moving them from member functions of > OBAtom/OBBond to static functions, e.g. from OBAtom::IsXXXX() to > static OBAtom_IsXXXX(OBAtom*). Functionally everything should be the > same. > > Regards, > - Noel > > On 27 May 2016 at 19:47, Stefano Forli <[hidden email]> wrote: >> I welcome this discussion because it addresses some of the issues I had when >> starting contributing to the code. >> >> I'm partially responsible of several convenience functions (IsSulfoneOxygen, >> to name one :), and I would be happy to have a guideline to follow to deal >> with the issue. >> >> The rationale is: I've been schooled by experienced programmers to keep >> functions as simple as possible in order to limit bugs (like that rock band >> in which they paint their faces black and white). >> >> Prior to my patch, IsHbondAcceptor was just a handful of lines, and adding >> features made it grow to several 'pages' of source. Adding convenience >> functions was an attempt to improve code clarity and reduce chances of bugs. >> Although, I agree with Noel in terms of not creating ad-hoc functions which >> are used only once. If there's a way to group them together to keep them >> isolated from the main code (or not public?), I'm fine with it. >> I'm a big fan of generalizations over special cases. >> >> A similar thing is happening for the OBBond::IsEster function, but in this >> case I don't know how to deal with new features. >> I've tried fixing a possible bug and extend the functionalities, so the code >> went from 29 to 296 lines (including some documentation/comments). >> In order to cover all esters accordingly to IUPAC (and Wikipedia, of >> course), I ended up in writing extra functions that are called by IsEster >> itself, but can still be called independently: >> >> IsEster: >> ->IsPhosphateEster >> ->IsCarboxyEster >> ->IsThioEster >> >> As a consequence, the ABI would change too, and IsEster now could accept >> several extra boolean flags to deal with these extra features (before it >> accepted none). >> >> I think these extra functions could be considered convenience functions. In >> principle, the whole code could be in a single, larger function, which will >> have one well-defined behavior. >> >> Although, I think this would imply the inability to customize the behavior >> of IsEster, and limit the access to new features. >> >> In a possible scenario, a user could call IsEster(), and then write its own >> code to get discriminate between the different ester types. Since the >> function already did the job, why not make it available directly? >> >> I'm aware that this is a complex reply to what was a simple and well-posed >> point, but I would appreciate to have such aspects clarified to prevent bad >> coding practices to sneak in the project. >> >> It would be great if eventually such guidelines would land in a nice and >> small "Guidelines" page for OB developers and contributors. >> >> Thanks! >> >> S >> >> >> >> >> On 05/27/2016 08:24 AM, Noel O'Boyle wrote: >>> >>> I was just going to make the point that I'm not too keen on the >>> majority of the additions to OBAtom and OBMol, which appear to be >>> convenience functions. Consider IsSulfoneOxygen. There are a lot of >>> elements out there and a lot of functional groups; we should either >>> add them all or add none. If in the end, this function is absolutely >>> necessary, surely such convenience functions could live elsewhere >>> rather than being in the central classes of the library. >>> >>> I blame IsHydrogen() - it's a magnet for further "Is"s :-) >>> >>> - Noel >>> >>> On 23 May 2016 at 19:06, Geoffrey Hutchison <[hidden email]> >>> wrote: >>>>> >>>>> >>>>> http://baoilleach.webfactional.com/site_media/ob_api_check/compat_report_21052016.html >>>>> It makes a few mistakes here and there but is accurate in general. >>>> >>>> >>>> It's a bit weird about adding optional parameters, e.g. >>>> bool Build(OBMol &mol); >>>> becomes >>>> bool Build(OBMol &mol, bool stereoWarnings = true); >>>> >>>> This isn't an API issue, since a program build to the first will still >>>> recompile. This is an ABI problem, but we've never been too concerned about >>>> that from 2.x to 2.y. >>>> >>>>> I have a few comments to make regarding some of the changes, but will do >>>>> that separately. >>>> >>>> >>>> I'm obviously interested in the feedback. It's also useful to make sure >>>> that source documentation was added. >>>> >>>> Thanks, >>>> -Geoff >>> >>> >>> >>> ------------------------------------------------------------------------------ >>> What NetFlow Analyzer can do for you? Monitors network bandwidth and >>> traffic >>> patterns at an interface-level. Reveals which users, apps, and protocols >>> are >>> consuming the most bandwidth. Provides multi-vendor support for NetFlow, >>> J-Flow, sFlow and other flows. Make informed decisions using capacity >>> planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e >>> _______________________________________________ >>> OpenBabel-Devel mailing list >>> [hidden email] >>> https://lists.sourceforge.net/lists/listinfo/openbabel-devel >>> >> >> -- >> >> Stefano Forli, PhD >> >> Assistant Professor of Integrative >> Structural and Computational Biology, >> Molecular Graphics Laboratory >> >> Dept. of Integrative Structural >> and Computational Biology, MB-112A >> The Scripps Research Institute >> 10550 North Torrey Pines Road >> La Jolla, CA 92037-1000, USA. >> >> tel: +1 (858)784-2055 >> fax: +1 (858)784-2860 >> email: [hidden email] >> http://www.scripps.edu/~forli/ ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e _______________________________________________ OpenBabel-Devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/openbabel-devel |
In reply to this post by Noel O'Boyle
On 02/06/16 08:33, Noel O'Boyle wrote:
> Hi Stefano, > > Short functions are good - I should really be better at this - but > there is no need to add all of your functions to the public API. From > what you say, it sounds like this was unintentional. In that case, is > it okay if I change things to remove these utility functions from the > public API? This will involve moving them from member functions of > OBAtom/OBBond to static functions, e.g. from OBAtom::IsXXXX() to > static OBAtom_IsXXXX(OBAtom*). Functionally everything should be the > same. > > Regards, > - Noel > > On 27 May 2016 at 19:47, Stefano Forli <[hidden email]> wrote: >> I welcome this discussion because it addresses some of the issues I had when >> starting contributing to the code. >> >> I'm partially responsible of several convenience functions (IsSulfoneOxygen, >> to name one :), and I would be happy to have a guideline to follow to deal >> with the issue. >> >> The rationale is: I've been schooled by experienced programmers to keep >> functions as simple as possible in order to limit bugs (like that rock band >> in which they paint their faces black and white). >> >> Prior to my patch, IsHbondAcceptor was just a handful of lines, and adding >> features made it grow to several 'pages' of source. Adding convenience >> functions was an attempt to improve code clarity and reduce chances of bugs. >> Although, I agree with Noel in terms of not creating ad-hoc functions which >> are used only once. If there's a way to group them together to keep them >> isolated from the main code (or not public?), I'm fine with it. >> I'm a big fan of generalizations over special cases. >> >> A similar thing is happening for the OBBond::IsEster function, but in this >> case I don't know how to deal with new features. >> I've tried fixing a possible bug and extend the functionalities, so the code >> went from 29 to 296 lines (including some documentation/comments). >> In order to cover all esters accordingly to IUPAC (and Wikipedia, of >> course), I ended up in writing extra functions that are called by IsEster >> itself, but can still be called independently: >> >> IsEster: >> ->IsPhosphateEster >> ->IsCarboxyEster >> ->IsThioEster >> >> As a consequence, the ABI would change too, and IsEster now could accept >> several extra boolean flags to deal with these extra features (before it >> accepted none). >> >> I think these extra functions could be considered convenience functions. In >> principle, the whole code could be in a single, larger function, which will >> have one well-defined behavior. >> >> Although, I think this would imply the inability to customize the behavior >> of IsEster, and limit the access to new features. >> >> In a possible scenario, a user could call IsEster(), and then write its own >> code to get discriminate between the different ester types. Since the >> function already did the job, why not make it available directly? >> >> I'm aware that this is a complex reply to what was a simple and well-posed >> point, but I would appreciate to have such aspects clarified to prevent bad >> coding practices to sneak in the project. >> >> It would be great if eventually such guidelines would land in a nice and >> small "Guidelines" page for OB developers and contributors. >> >> Thanks! >> >> S >> >> >> >> >> On 05/27/2016 08:24 AM, Noel O'Boyle wrote: >>> >>> I was just going to make the point that I'm not too keen on the >>> majority of the additions to OBAtom and OBMol, which appear to be >>> convenience functions. Consider IsSulfoneOxygen. There are a lot of >>> elements out there and a lot of functional groups; we should either >>> add them all or add none. If in the end, this function is absolutely >>> necessary, surely such convenience functions could live elsewhere >>> rather than being in the central classes of the library. >>> >>> I blame IsHydrogen() - it's a magnet for further "Is"s :-) >>> >>> - Noel >>> >>> On 23 May 2016 at 19:06, Geoffrey Hutchison <[hidden email]> >>> wrote: >>>>> >>>>> >>>>> http://baoilleach.webfactional.com/site_media/ob_api_check/compat_report_21052016.html >>>>> It makes a few mistakes here and there but is accurate in general. >>>> >>>> >>>> It's a bit weird about adding optional parameters, e.g. >>>> bool Build(OBMol &mol); >>>> becomes >>>> bool Build(OBMol &mol, bool stereoWarnings = true); >>>> >>>> This isn't an API issue, since a program build to the first will still >>>> recompile. This is an ABI problem, but we've never been too concerned about >>>> that from 2.x to 2.y. >>>> >>>>> I have a few comments to make regarding some of the changes, but will do >>>>> that separately. >>>> >>>> >>>> I'm obviously interested in the feedback. It's also useful to make sure >>>> that source documentation was added. >>>> >>>> Thanks, >>>> -Geoff >>> >>> >>> >>> ------------------------------------------------------------------------------ >>> What NetFlow Analyzer can do for you? Monitors network bandwidth and >>> traffic >>> patterns at an interface-level. Reveals which users, apps, and protocols >>> are >>> consuming the most bandwidth. Provides multi-vendor support for NetFlow, >>> J-Flow, sFlow and other flows. Make informed decisions using capacity >>> planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e >>> _______________________________________________ >>> OpenBabel-Devel mailing list >>> [hidden email] >>> https://lists.sourceforge.net/lists/listinfo/openbabel-devel >>> >> >> -- >> >> Stefano Forli, PhD >> >> Assistant Professor of Integrative >> Structural and Computational Biology, >> Molecular Graphics Laboratory >> >> Dept. of Integrative Structural >> and Computational Biology, MB-112A >> The Scripps Research Institute >> 10550 North Torrey Pines Road >> La Jolla, CA 92037-1000, USA. >> >> tel: +1 (858)784-2055 >> fax: +1 (858)784-2860 >> email: [hidden email] >> http://www.scripps.edu/~forli/ > > ------------------------------------------------------------------------------ > What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic > patterns at an interface-level. Reveals which users, apps, and protocols are > consuming the most bandwidth. Provides multi-vendor support for NetFlow, > J-Flow, sFlow and other flows. Make informed decisions using capacity > planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e > _______________________________________________ > OpenBabel-Devel mailing list > [hidden email] > https://lists.sourceforge.net/lists/listinfo/openbabel-devel > -- David van der Spoel, Ph.D., Professor of Biology Dept. of Cell & Molec. Biol., Uppsala University. Box 596, 75124 Uppsala, Sweden. Phone: +46184714205. [hidden email] http://folding.bmc.uu.se ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e _______________________________________________ OpenBabel-Devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/openbabel-devel |
Administrator
|
The thing is, if I was matching a functional group, I would just use a
SMARTS pattern. This is a general solution for 99% of cases, rather than having a function for each one. We have a set of SMARTS patterns for functional groups in the .txt file for FP3. The added benefit of a SMARTS pattern is that you have the match of each atom to each virtual atom of the pattern. For a function of OBAtom, it's just boolean true or false. - Noel On 2 June 2016 at 07:37, David van der Spoel <[hidden email]> wrote: > On 02/06/16 08:33, Noel O'Boyle wrote: >> Hi Stefano, >> >> Short functions are good - I should really be better at this - but >> there is no need to add all of your functions to the public API. From >> what you say, it sounds like this was unintentional. In that case, is >> it okay if I change things to remove these utility functions from the >> public API? This will involve moving them from member functions of >> OBAtom/OBBond to static functions, e.g. from OBAtom::IsXXXX() to >> static OBAtom_IsXXXX(OBAtom*). Functionally everything should be the >> same. >> > How about combining that with my proposal? > >> Regards, >> - Noel >> >> On 27 May 2016 at 19:47, Stefano Forli <[hidden email]> wrote: >>> I welcome this discussion because it addresses some of the issues I had when >>> starting contributing to the code. >>> >>> I'm partially responsible of several convenience functions (IsSulfoneOxygen, >>> to name one :), and I would be happy to have a guideline to follow to deal >>> with the issue. >>> >>> The rationale is: I've been schooled by experienced programmers to keep >>> functions as simple as possible in order to limit bugs (like that rock band >>> in which they paint their faces black and white). >>> >>> Prior to my patch, IsHbondAcceptor was just a handful of lines, and adding >>> features made it grow to several 'pages' of source. Adding convenience >>> functions was an attempt to improve code clarity and reduce chances of bugs. >>> Although, I agree with Noel in terms of not creating ad-hoc functions which >>> are used only once. If there's a way to group them together to keep them >>> isolated from the main code (or not public?), I'm fine with it. >>> I'm a big fan of generalizations over special cases. >>> >>> A similar thing is happening for the OBBond::IsEster function, but in this >>> case I don't know how to deal with new features. >>> I've tried fixing a possible bug and extend the functionalities, so the code >>> went from 29 to 296 lines (including some documentation/comments). >>> In order to cover all esters accordingly to IUPAC (and Wikipedia, of >>> course), I ended up in writing extra functions that are called by IsEster >>> itself, but can still be called independently: >>> >>> IsEster: >>> ->IsPhosphateEster >>> ->IsCarboxyEster >>> ->IsThioEster >>> >>> As a consequence, the ABI would change too, and IsEster now could accept >>> several extra boolean flags to deal with these extra features (before it >>> accepted none). >>> >>> I think these extra functions could be considered convenience functions. In >>> principle, the whole code could be in a single, larger function, which will >>> have one well-defined behavior. >>> >>> Although, I think this would imply the inability to customize the behavior >>> of IsEster, and limit the access to new features. >>> >>> In a possible scenario, a user could call IsEster(), and then write its own >>> code to get discriminate between the different ester types. Since the >>> function already did the job, why not make it available directly? >>> >>> I'm aware that this is a complex reply to what was a simple and well-posed >>> point, but I would appreciate to have such aspects clarified to prevent bad >>> coding practices to sneak in the project. >>> >>> It would be great if eventually such guidelines would land in a nice and >>> small "Guidelines" page for OB developers and contributors. >>> >>> Thanks! >>> >>> S >>> >>> >>> >>> >>> On 05/27/2016 08:24 AM, Noel O'Boyle wrote: >>>> >>>> I was just going to make the point that I'm not too keen on the >>>> majority of the additions to OBAtom and OBMol, which appear to be >>>> convenience functions. Consider IsSulfoneOxygen. There are a lot of >>>> elements out there and a lot of functional groups; we should either >>>> add them all or add none. If in the end, this function is absolutely >>>> necessary, surely such convenience functions could live elsewhere >>>> rather than being in the central classes of the library. >>>> >>>> I blame IsHydrogen() - it's a magnet for further "Is"s :-) >>>> >>>> - Noel >>>> >>>> On 23 May 2016 at 19:06, Geoffrey Hutchison <[hidden email]> >>>> wrote: >>>>>> >>>>>> >>>>>> http://baoilleach.webfactional.com/site_media/ob_api_check/compat_report_21052016.html >>>>>> It makes a few mistakes here and there but is accurate in general. >>>>> >>>>> >>>>> It's a bit weird about adding optional parameters, e.g. >>>>> bool Build(OBMol &mol); >>>>> becomes >>>>> bool Build(OBMol &mol, bool stereoWarnings = true); >>>>> >>>>> This isn't an API issue, since a program build to the first will still >>>>> recompile. This is an ABI problem, but we've never been too concerned about >>>>> that from 2.x to 2.y. >>>>> >>>>>> I have a few comments to make regarding some of the changes, but will do >>>>>> that separately. >>>>> >>>>> >>>>> I'm obviously interested in the feedback. It's also useful to make sure >>>>> that source documentation was added. >>>>> >>>>> Thanks, >>>>> -Geoff >>>> >>>> >>>> >>>> ------------------------------------------------------------------------------ >>>> What NetFlow Analyzer can do for you? Monitors network bandwidth and >>>> traffic >>>> patterns at an interface-level. Reveals which users, apps, and protocols >>>> are >>>> consuming the most bandwidth. Provides multi-vendor support for NetFlow, >>>> J-Flow, sFlow and other flows. Make informed decisions using capacity >>>> planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e >>>> _______________________________________________ >>>> OpenBabel-Devel mailing list >>>> [hidden email] >>>> https://lists.sourceforge.net/lists/listinfo/openbabel-devel >>>> >>> >>> -- >>> >>> Stefano Forli, PhD >>> >>> Assistant Professor of Integrative >>> Structural and Computational Biology, >>> Molecular Graphics Laboratory >>> >>> Dept. of Integrative Structural >>> and Computational Biology, MB-112A >>> The Scripps Research Institute >>> 10550 North Torrey Pines Road >>> La Jolla, CA 92037-1000, USA. >>> >>> tel: +1 (858)784-2055 >>> fax: +1 (858)784-2860 >>> email: [hidden email] >>> http://www.scripps.edu/~forli/ >> >> ------------------------------------------------------------------------------ >> What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic >> patterns at an interface-level. Reveals which users, apps, and protocols are >> consuming the most bandwidth. Provides multi-vendor support for NetFlow, >> J-Flow, sFlow and other flows. Make informed decisions using capacity >> planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e >> _______________________________________________ >> OpenBabel-Devel mailing list >> [hidden email] >> https://lists.sourceforge.net/lists/listinfo/openbabel-devel >> > > > -- > David van der Spoel, Ph.D., Professor of Biology > Dept. of Cell & Molec. Biol., Uppsala University. > Box 596, 75124 Uppsala, Sweden. Phone: +46184714205. > [hidden email] http://folding.bmc.uu.se > > ------------------------------------------------------------------------------ > What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic > patterns at an interface-level. Reveals which users, apps, and protocols are > consuming the most bandwidth. Provides multi-vendor support for NetFlow, > J-Flow, sFlow and other flows. Make informed decisions using capacity > planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e > _______________________________________________ > OpenBabel-Devel mailing list > [hidden email] > https://lists.sourceforge.net/lists/listinfo/openbabel-devel ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e _______________________________________________ OpenBabel-Devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/openbabel-devel |
> SMARTS pattern is that you have the match of each atom to each virtual
> atom of the pattern. For a function of OBAtom, it's just boolean true > or false. Moreover, there's already OBAtom::MatchesSMARTS() http://openbabel.org/dev-api/classOpenBabel_1_1OBAtom.shtml#abfdefac42d3c895920f1f715f0d710a5 Some of these "one-off" functions are certainly more efficient, and I do like David's enum-based suggestion. My feeling is that we should decide fairly quickly on any API changes so that we can issue 2.4 soon. Thanks, -Geoff ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e _______________________________________________ OpenBabel-Devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/openbabel-devel |
Administrator
|
Hi Stefano,
Your comment about guidelines is of course true. I've been thinking about this. My feeling is that only needed functions should be in the API. So it's not that a function is not useful, or that someone might find it useful. This is not a concrete guideline of course, but it's something to bear in mind. Regards, - Noel On 2 June 2016 at 16:34, Geoffrey Hutchison <[hidden email]> wrote: >> SMARTS pattern is that you have the match of each atom to each virtual >> atom of the pattern. For a function of OBAtom, it's just boolean true >> or false. > > Moreover, there's already OBAtom::MatchesSMARTS() > http://openbabel.org/dev-api/classOpenBabel_1_1OBAtom.shtml#abfdefac42d3c895920f1f715f0d710a5 > > Some of these "one-off" functions are certainly more efficient, and I do like David's enum-based suggestion. > > My feeling is that we should decide fairly quickly on any API changes so that we can issue 2.4 soon. > > Thanks, > -Geoff ------------------------------------------------------------------------------ Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San Francisco, CA to explore cutting-edge tech and listen to tech luminaries present their vision of the future. This family event has something for everyone, including kids. Get more information and register today. http://sdm.link/attshape _______________________________________________ OpenBabel-Devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/openbabel-devel |
Noel,
thanks for the reply, I had some time to think about your email and the issue, here are a few points to discuss. I like the concept of necessity could be the seed around which guidelines could be built... once we define explicitly what "necessary" means. An initial structure for the guidelines could be: 1. Define what criteria would we use to consider a function necessary. Once that's clarified, it should be easier to prevent unnecessary changes to the API's. 2. If an API gets modified and becomes more complex (i.e., more optional parameters?) defaults should be set such that the new behavior mimics the old API. 3. Functions should be written to be small, and do a single operation while keeping the code simple. If the function code gets too large, it should be broken down into smaller, helper functions. These helper functions should be reusable, but kept private as much as possible, preventing the proliferation of new API's. 4. Some code styling should also be enforced. I found the idTech 4 coding standard (used by id software [1,2]) to be incredibly useful and a remarkable source of inspiration. This should be extended to the specific needs of OpenBabel. For example, encourage using iterators in macros vs building our own. Not sure if this is really important, and I've seen using both ways indiscriminately (I suspect geological stratification of new code over old one), but I think would be easier on the code base, preventing the reinvention of the wheel and the potential inclusion of new bugs. 5. I have no experience whatsoever on compilation, warnings and such, but given the recent discussion in the mailing list, I believe it would be good to provide a clear and standard set of options needed to compile the code in way that makes it acceptable for incorporation in the main branch and redistribution. 6. Documentation. If new code is added, it *must* be documented. I found very hard (for a rookie, though) to understand how documentation is added and maintained. I believe there's a way to generate documentation directly from comments in the code, and I will be really happy to be able to do that. [1] http://kotaku.com/5975610/the-exceptional-beauty-of-doom-3s-source-code [2] ftp://ftp.idsoftware.com/idstuff/doom3/source/codestyleconventions.doc Needless to say, all these points are just suggestions. There's a lot more to be said on the matter, and I trust more experienced coders to improve this stub. Best, S -- Stefano Forli, PhD Assistant Professor of Integrative Structural and Computational Biology, Molecular Graphics Laboratory Dept. of Integrative Structural and Computational Biology, MB-112F The Scripps Research Institute 10550 North Torrey Pines Road La Jolla, CA 92037-1000, USA. tel: +1 (858)784-2055 fax: +1 (858)784-2860 email: [hidden email] http://www.scripps.edu/~forli/ ________________________________________ From: Noel O'Boyle [[hidden email]] Sent: Tuesday, June 21, 2016 12:16 PM To: Geoffrey Hutchison Cc: David van der Spoel; Openbabel-DEV; Stefano Forli Subject: Re: [OpenBabel-Devel] OB API/ABI changes Hi Stefano, Your comment about guidelines is of course true. I've been thinking about this. My feeling is that only needed functions should be in the API. So it's not that a function is not useful, or that someone might find it useful. This is not a concrete guideline of course, but it's something to bear in mind. Regards, - Noel On 2 June 2016 at 16:34, Geoffrey Hutchison <[hidden email]> wrote: >> SMARTS pattern is that you have the match of each atom to each virtual >> atom of the pattern. For a function of OBAtom, it's just boolean true >> or false. > > Moreover, there's already OBAtom::MatchesSMARTS() > http://openbabel.org/dev-api/classOpenBabel_1_1OBAtom.shtml#abfdefac42d3c895920f1f715f0d710a5 > > Some of these "one-off" functions are certainly more efficient, and I do like David's enum-based suggestion. > > My feeling is that we should decide fairly quickly on any API changes so that we can issue 2.4 soon. > > Thanks, > -Geoff ------------------------------------------------------------------------------ Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San Francisco, CA to explore cutting-edge tech and listen to tech luminaries present their vision of the future. This family event has something for everyone, including kids. Get more information and register today. http://sdm.link/attshape _______________________________________________ OpenBabel-Devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/openbabel-devel |
Administrator
|
I have thought about writing up a section of the documentation focussed on developers. Too many guidelines and rules turns people off though and we really want to encourage new devs and not create barriers. Experienced programmers will look at the existing code and adapt to the existing coding style. New ones may not appreciate being informed about style guidelines when this is their first C++ code. There's a balance there of course. I would love every pull request to be accompanied by docs and tests. Enforcing this is a different matter. We do this in cclib as we have had a test driven culture from the start and all of the devs can see the value. It could be that we don't enforce it for the first few pull requests but require it after that - I.e. have a grace period. By the way, the api docs are built with "make docs" I believe. Naturally this is not documented :-) On 26 Jun 2016 8:46 p.m., "Stefano Forli" <[hidden email]> wrote:
Noel, ------------------------------------------------------------------------------ Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San Francisco, CA to explore cutting-edge tech and listen to tech luminaries present their vision of the future. This family event has something for everyone, including kids. Get more information and register today. http://sdm.link/attshape _______________________________________________ OpenBabel-Devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/openbabel-devel |
In reply to this post by Noel O'Boyle
Hi,
On Sun, May 22, 2016 at 10:00:30PM +0100, Noel O'Boyle wrote: > To help with getting the release together, I've run the ABI compliance > checker on the current dev code versus 2.3.2. > > The result is available at: > http://baoilleach.webfactional.com/site_media/ob_api_check/compat_report_21052016.html Jumping on this thread, but. What I find weird is that the 2.4 library version is 4.0.0 while the 2.3.2 library version was 4.0.2, i.e. higher. At least the above link lists a ton of removed symbols, so shouldn't the library version have been bumped to 5.0.0? Michael ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot _______________________________________________ OpenBabel-Devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/openbabel-devel |
> What I find weird is that the 2.4 library version is 4.0.0 while the
> 2.3.2 library version was 4.0.2, i.e. higher. I thought I replied to this, but my e-mail program is indicating no reply. This was a mistake. I'm not sure why the "master" branch didn't update the library version, so I left it (incorrectly) as 4.0.0. I've now bumped it in GitHub as 5.0.0 and will release a 2.4.1 release to cover this and a few other minor issues. -Geoff ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot _______________________________________________ OpenBabel-Devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/openbabel-devel |
Free forum by Nabble | Edit this page |