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 <sp...@xray.bmc.uu.se> 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 <fo...@scripps.edu> 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 <geoff.hutchi...@gmail.com> >>>> 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 >>>> OpenBabel-Devel@lists.sourceforge.net >>>> 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: fo...@scripps.edu >>> 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 >> OpenBabel-Devel@lists.sourceforge.net >> 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. > sp...@xray.bmc.uu.se 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 > OpenBabel-Devel@lists.sourceforge.net > 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 OpenBabel-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openbabel-devel
