Hi Noel,
I’m mostly just grumpy because these changes broke my build. Have you
considered going through a deprecated phase?
From your email, I thought you were saying you added an IsElement method to
OBAtom, but this doesn’t seem to be the case. Something like:
OBAtom.IsElement(OBElement::Hydrogen)
is quite readable and it’s much more general than a specific helper function.
There’s also no need worry about subtle bugs incurred from incorrectly
accounting for operator precedence (e.g., the == in GetAtomicNum() == 1).
You could even shorten the name to “Is”:
atom->Is(OBElement::Hydrogen)
David Koes
Assistant Professor
Computational & Systems Biology
University of Pittsburgh
On Jan 12, 2018, at 9:01 AM, Noel O'Boyle
<baoille...@gmail.com<mailto:baoille...@gmail.com>> wrote:
Hi David,
We don't break backwards compatability lightly. The last time was 12 years ago.
In that time, there has been some accumulation of cruft, and in addition there
are some necessary changes in order to fix underlying problems.
Regarding the specific functions you mention, the IsElement() functions are, as
you say, convenience functions, some of which date back to the original
Stahl/Walters Babel. While GetAtomicNumber() == 1 is less readable and error
prone as you say, I have added the elements as enums, OBElement::Hydrogen, to
mitigate this. Furthermore, while the IsElement() methods only covered a small
number of elements, the ones where people are most likely to know the atomic
number (where's IsFluorine() for instance), the OBElement namespace covers them
all.
So why have I removed them? They are convenience functions trivially
implementable in terms of the remaining API. There is nothing to prevent a user
implementing the function themselves as a macro or whatever. From the port of
Open Babel to remove these function, I can give examples of where the use of
these functions results in inefficient code. Obviously there are two function
lookups instead of one. But also, I have seen constructions like "if
atom->IsHydrogen then this; else if atom->IsCarbon then this; and so on. In
other words, instead of getting the atomic number once and switching on it, the
maximum possible amount of work is done.
Regarding IsSingle(), the situation is more clear cut. This is a function which
appears to be (and is documented to be) synonymous with GetBondOrder() == 1.
Aha, but it is not, and only by looking at the source code would you have
realised the difference. It was a surprise to me, and I think it is used
synonymous with GetBondOrder==1 throughout the codebase...but there's no way to
tell what the caller intended. All we can do is prevent nasty surprises for
users in the future. This change was made in combination with an overhaul of
the treatment of kekulization and aromaticity last year.
Regards,
- Noel
------------------------------------------------------------------------------
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
OpenBabel-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
