Re: [OpenBabel-Devel] Release 3.0

[Geoffrey Hutchison]

> Is there any work planned for that?

At the moment, no. As you know, it's certainly do-able, but I think the main 
question is "what's the use case." (I'm getting more focused in my old age, I 
guess.)

> A while back I looked at the code trying to fix a few problems (chirality of 
> protonated nitrogens, to name one), but I couldn't understand the code well 
> enough to do anything useful.

If you can find the particular items of confusion, that would be great.

> Also, in my lab we've done some work on the hydrogen bond typing, following 
> Noel's advice on using SMARTS instead of convoluted functions for different 
> chemical group. I have to double-check the status of that code, but if you 
> think it's worth looking at, I'll be happy to contribute.

Yes. While Noel has done work to minimize the amount of SMARTS used in routine 
transformations, I think SMARTS is an ideal platform for things like atom and 
bond typing.

-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
OpenBabel-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openbabel-devel

Re: [OpenBabel-Devel] Release 3.0

[Stefano Forli]

I was going to comment on the process, but I got sidetracked.

I've just remembered because I've just seen a question in the user mailing list about the 
stereoisomery and the enumeration of stereo-configurations.


Is there any work planned for that?
A while back I looked at the code trying to fix a few problems (chirality of protonated 
nitrogens, to name one), but I couldn't understand the code well enough to do anything useful.


Also, in my lab we've done some work on the hydrogen bond typing, following Noel's advice 
on using SMARTS instead of convoluted functions for different chemical group. I have to 
double-check the status of that code, but if you think it's worth looking at, I'll be 
happy to contribute.


S

On 01/12/2018 08:45 AM, Geoffrey Hutchison wrote:

I’m mostly just grumpy because these changes broke my build. Have you 
considered
going through a deprecated phase? 



While I appreciate the grumpiness, you don't have to break a build - the "master" branch 
is naturally going to have changes. I'd like to think Noel (and I) were pretty clear with 
discussion on-list and on GitHub going back into the summer and fall. The specific change 
that you mention seems to relate to:

https://github.com/openbabel/openbabel/pull/1601

This was discussed in June-July and merged in August.


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.


In general, Noel has been undertaking a significant effort to clean up API calls to match 
"least surprise."


Thus, the version number has been bumped. This release makes a clear effort to indicate 
"Hey, something big is different - it's version 3!"


As to a generic OBAtom::IsElement(OBElement::Hydrogen) call, I look forward to a pull 
request. I'm not sure I'll have time to do it myself today, but:

- We haven't released v3 yet, so we appreciate concern and comments
- I bumped the version number to indicate backwards-incompatibility
- Yes, migration notes would be helpful, but aren't done yet

If you don't like the changes, I suggest using a release branch:
https://github.com/openbabel/openbabel/tree/openbabel-2-4-x

-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
OpenBabel-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openbabel-devel



--

 Stefano Forli, PhD

 Assistant Professor
 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/


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

Re: [OpenBabel-Devel] Release 3.0

[Geoffrey Hutchison]

> I’m mostly just grumpy because these changes broke my build. Have you 
> considered going through a deprecated phase?  


While I appreciate the grumpiness, you don't have to break a build - the 
"master" branch is naturally going to have changes. I'd like to think Noel (and 
I) were pretty clear with discussion on-list and on GitHub going back into the 
summer and fall. The specific change that you mention seems to relate to:
https://github.com/openbabel/openbabel/pull/1601

This was discussed in June-July and merged in August.

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

In general, Noel has been undertaking a significant effort to clean up API 
calls to match "least surprise."

Thus, the version number has been bumped. This release makes a clear effort to 
indicate "Hey, something big is different - it's version 3!"

As to a generic OBAtom::IsElement(OBElement::Hydrogen) call, I look forward to 
a pull request. I'm not sure I'll have time to do it myself today, but:
- We haven't released v3 yet, so we appreciate concern and comments
- I bumped the version number to indicate backwards-incompatibility
- Yes, migration notes would be helpful, but aren't done yet

If you don't like the changes, I suggest using a release branch:
https://github.com/openbabel/openbabel/tree/openbabel-2-4-x

-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
OpenBabel-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openbabel-devel

Re: [OpenBabel-Devel] Release 3.0

[Noel O'Boyle]

Hi David,

I'd love to have the time to do everything perfectly. Yes, we should have a
deprecation phase, with compiler warnings, and messages generated by the
bindings. But eventually, the changes will still break the build. What we
do need are migration docs at the least.

Regards,
- Noel

On 12 January 2018 at 14:43, Koes, David  wrote:

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

Re: [OpenBabel-Devel] Release 3.0

[Koes, David]

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

Re: [OpenBabel-Devel] Release 3.0

[Noel O'Boyle]

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

On 11 January 2018 at 21:26, David Koes  wrote:

> I'd like to advocate for _not_ removing simple convenience functions like
> OBAtom->IsHydrogen, OBBond->IsSingle.  I don't see any benefit: it breaks
> backwards compatibility, using GetAtomicNumber() == 1 is less readable and
> more error prone, and GetBondOrder() == 1 doesn't have the same semantics
> as IsSingle did.
>
> My two cents.
>
> David Koes
>
> Assistant Professor
> Computational & Systems Biology
> University of Pittsburgh
>
> 
> --
> 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
>
--
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

Re: [OpenBabel-Devel] Release 3.0

[David Koes]

I'd like to advocate for _not_ removing simple convenience functions like OBAtom->IsHydrogen, OBBond->IsSingle.  I don't 
see any benefit: it breaks backwards compatibility, using GetAtomicNumber() == 1 is less readable and more error prone, 
and GetBondOrder() == 1 doesn't have the same semantics as IsSingle did.


My two cents.

David Koes

Assistant Professor
Computational & Systems Biology
University of Pittsburgh

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

Re: [OpenBabel-Devel] Release 3.0

[Geoffrey Hutchison]

If you don't mind, I'm putting your message onto the developer list.

I initiated an OB-3.0 release plan on GitHub because I think the changes you've 
made so far are significant and backwards-incompatible. I think that's really 
worth signaling.
(As in, if you have code that uses OB-2.x APIs, you'll probably need to do at 
least some updating.)

So yes, while I didn't have a chance to say "2018 Plans" yet, I'd like to see 
some discussion on when 3.0 should be released and any remaining projects.

I think the projects you mention are good.

Other ideas? (Opening this to the list..)

-Geoff


> On Jan 7, 2018, at 3:45 PM, Noel O'Boyle  wrote:
> 
> Hey Geoff,
> 
> Happy New Year and all that.
> 
> I saw the github project for release 3.0, and so I've been thinking about 
> whether I might be able to make additional changes before the API is set in 
> stone.
> 
> I don't know what timeframe you had in mind but here are a couple of things I 
> wouldn't mind doing before then...
> 1. Refactoring reaction handling.
> 2. Removing convenience functions.
> 3. Removing vestiges of the original stereo handling that no longer apply 
> (like one or two functions and maybe a flag or two)
> 4. Tidy up the valence methods, e.g. OBAtom::GetValence() does not return the 
> valence, nor is there any function that does.
> 
> And that's basically it. I need to look into (1) to figure out how much work 
> it is (and write up the proposal), but the others are fairly small though (2) 
> and (4) obviously require some discussion.
> 
> So what do you think? Had enough of changes, or willing to hold off for a 
> while? Or have any other ideas?
> 
> 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