OB API/ABI changes

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

OB API/ABI changes

Noel O'Boyle
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
Reply | Threaded
Open this post in threaded view
|

Re: OB API/ABI changes

Geoff Hutchison
> 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
Reply | Threaded
Open this post in threaded view
|

Re: OB API/ABI changes

Noel O'Boyle
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
Reply | Threaded
Open this post in threaded view
|

Re: OB API/ABI changes

Stefano Forli
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
Reply | Threaded
Open this post in threaded view
|

Re: OB API/ABI changes

David van der Spoel
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.
>
The simplest way around this which would not change the API more than
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
Reply | Threaded
Open this post in threaded view
|

Re: OB API/ABI changes

Noel O'Boyle
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
Reply | Threaded
Open this post in threaded view
|

Re: OB API/ABI changes

Noel O'Boyle
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
Reply | Threaded
Open this post in threaded view
|

Re: OB API/ABI changes

David van der Spoel
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.
>
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
Reply | Threaded
Open this post in threaded view
|

Re: OB API/ABI changes

Noel O'Boyle
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
Reply | Threaded
Open this post in threaded view
|

Re: OB API/ABI changes

Geoff Hutchison
> 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
Reply | Threaded
Open this post in threaded view
|

Re: OB API/ABI changes

Noel O'Boyle
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
Reply | Threaded
Open this post in threaded view
|

Re: OB API/ABI changes

Stefano Forli
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
Reply | Threaded
Open this post in threaded view
|

Re: OB API/ABI changes

Noel O'Boyle
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,

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
Reply | Threaded
Open this post in threaded view
|

Re: OB API/ABI changes

Michael Banck-2
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
Reply | Threaded
Open this post in threaded view
|

Re: OB API/ABI changes

Geoff Hutchison
> 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