Re: [OpenBabel-Devel] [PATCH] Fix [#1v2] hydrogen valence calculation
> I've discovered a subtle bug in the calculation of OBAtom::KBOSum(), > specifically in OBAtom::GetImplicitValence, that causes the incorrect > value to be returned for hydrogens with valence greater than one. Thanks for this -- it's clear the bug can show up in other cases (e.g., hypervalent compounds, bridging halides, etc.), so it's a good bug fix beyond hydrogen. Thanks, -Geoff -- Cloud Services Checklist: Pricing and Packaging Optimization This white paper is intended to serve as a reference, checklist and point of discussion for anyone considering optimizing the pricing and packaging model of a cloud services business. Read Now! http://www.accelacomm.com/jaw/sfnl/114/51491232/ ___ OpenBabel-Devel mailing list OpenBabel-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Re: [OpenBabel-Devel] [PATCH] Inefficient idiom with STL empty() and size()
> and so on. There was once a time I'd have fixed this is g++, but I'm > getting old. Hopefully, there's a willing volunteer to audit and fix the > source tree. Well, I got the low-hanging fruit with a "grep" test: % grep 'empty()' *.cpp | grep 'size()' The benefit of fixing this in our source code is improved readability and improvement in Clang, Windows, etc. Perhaps it's worth sending this to cppcheck or another similar static analysis engine. -Geoff -- Cloud Services Checklist: Pricing and Packaging Optimization This white paper is intended to serve as a reference, checklist and point of discussion for anyone considering optimizing the pricing and packaging model of a cloud services business. Read Now! http://www.accelacomm.com/jaw/sfnl/114/51491232/ ___ OpenBabel-Devel mailing list OpenBabel-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Re: [OpenBabel-Devel] Simplying (and fixing) bindings
2011/12/7 Noel O'Boyle : > I'm afraid, it's not possible to ask disutils the location of the > site-packages folder (believe me, I have tried, googled, everything). > As a result, we have no choice. In any case, this simplifies > everything and I wouldn't change it back now. The user just needs to > add the lib directory to PYTHONPATH, RUBYPATH, CLASSPATH, PERL5LIB, > etc. http://docs.python.org/install/index.html#how-installation-works Just ask Python what its prefix (sys.prefix) and version is and you get the location of site-packages: prefix/lib/pythonX.Y/site-packages Some additional logic might be needed for multilib systems. Any reason why this can't be used? I don't know about other bindings, but there should be a way also. Reinis -- Cloud Services Checklist: Pricing and Packaging Optimization This white paper is intended to serve as a reference, checklist and point of discussion for anyone considering optimizing the pricing and packaging model of a cloud services business. Read Now! http://www.accelacomm.com/jaw/sfnl/114/51491232/ ___ OpenBabel-Devel mailing list OpenBabel-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openbabel-devel
[OpenBabel-Devel] [PATCH] Inefficient idiom with STL empty() and size()
This minor clean-up provides a negligible performance improvement
to OBAtom::GetValence(), but addresses a commonly occurring but
poor C++/STL programming idiom in the OpenBabel source tree.
The inefficient idiom that's currently used looks like:
return _vbond.empty() ? 0 : _vbond.size();
Whilst it is true for many STL data structures that testing
empty() is often more efficient than testing size() == 0, it is
rarely beneficial to use empty() purely to avoid calling size()
as a special case. This is certainly the case with std::vector
implementations on modern hardware.
To demonstrate this, consider the following test code
#include
std::vector v;
unsigned int oldway() {
return v.empty() ? 0 : static_cast(v.size());
}
unsigned int newway() {
return (unsigned int)v.size();
}
Using g++ v4.6.0 on x86_64, the original implementation requires
10 (9) instructions including a conditional jump
oldway: movqv+8(%rip), %rcx
movqv(%rip), %rdx
xorl%eax, %eax
cmpq%rdx, %rcx
je done
movq%rcx, %rax
subq%rdx, %rax
shrq$3, %rax
done: rep ; ret
whereas the obvious simpler idiom requires only 4 instructions
newway: movqv+8(%rip), %rax
subqv(%rip), %rax
sarq$3, %rax
ret
It turns out that this idiom/misunderstanding occurs in various places
throughout the source tree. In the attached patch I've fixed
GetValence()
[as I was investigating a valence-related SMARTS bug] and rotor.h as
another
occurrence in the same directory. However many examples still remain in
the main src/ directories including
typer.cpp:if (vs.empty() || vs.size() < 3)
rotor.cpp:if (!vs.empty() && vs.size() > 5)
phmodel.cpp:if (vs.empty() || vs.size() < 5)
mol.cpp:if (_vatom.empty() || _natoms+1 >=
(signed)_vatom.size())
and so on. There was once a time I'd have fixed this is g++, but I'm
getting old. Hopefully, there's a willing volunteer to audit and fix
the
source tree.
The following has been tested with "make test" with no regressions.
Index: include/openbabel/atom.h
===
*** include/openbabel/atom.h(revision 4680)
--- include/openbabel/atom.h(working copy)
*** namespace OpenBabel
*** 227,236
//! \deprecated Use GetCoordinateIdx() instead
unsigned int GetCIdx() const { return((int)_cidx); }
//! \return The current number of explicit connections
! unsigned int GetValence() const
! {
! return((_vbond.empty()) ? 0 : static_cast
(_vbond.size()));
! }
//! \return The hybridization of this atom: 1 for sp, 2 for sp2, 3 for
sp3, 4 for sq. planar, 5 for trig. bipy, 6 for octahedral
unsigned int GetHyb() const;
//! \return The implicit valence of this atom type (i.e. maximum number
of connections expected)
--- 227,233
//! \deprecated Use GetCoordinateIdx() instead
unsigned int GetCIdx() const { return((int)_cidx); }
//! \return The current number of explicit connections
! unsigned int GetValence() const { return (unsigned int)_vbond.size(); }
//! \return The hybridization of this atom: 1 for sp, 2 for sp2, 3 for
sp3, 4 for sq. planar, 5 for trig. bipy, 6 for octahedral
unsigned int GetHyb() const;
//! \return The implicit valence of this atom type (i.e. maximum number
of connections expected)
Index: include/openbabel/rotor.h
===
*** include/openbabel/rotor.h (revision 4680)
--- include/openbabel/rotor.h (working copy)
*** namespace OpenBabel
*** 467,473
*/
size_t Size()
{
! return((_rotor.empty()) ? (size_t)0: _rotor.size());
}
/**
* When no atoms/bonds are fixed or when bonds are fixed, this function
will
--- 467,473
*/
size_t Size()
{
! return _rotor.size();
}
/**
* When no atoms/bonds are fixed or when bonds are fixed, this function
will
Thanks in advance,
Roger
--
Roger Sayle, Ph.D.
CEO and founder
NextMove Software Ltd
Cambridge, UK
--
Cloud Services Checklist: Pricing and Packaging Optimization
This white paper is intended to serve as a reference, checklist and point of
discussion for anyone considering optimizing the pricing and packaging model
of a cloud services business. Read Now!
http://www.accelacomm.com/jaw/sfnl/114/51491232/___
OpenBabel-Devel mailing list
OpenBabel-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
[OpenBabel-Devel] [PATCH] Fix [#1v2] hydrogen valence calculation
I've discovered a subtle bug in the calculation of OBAtom::KBOSum(),
specifically in OBAtom::GetImplicitValence, that causes the incorrect
value to be returned for hydrogens with valence greater than one.
This manifests itself as the failure of obgrep to match the SMARTS
[v2] against the molecule C[H]C. Without this OpenBabel's SMARTS
can't be used to identify molecules with dubious hydrogen valences.
The problem is fixed by the attached patch which corrects the special
case for hydrogen in OBAtom::GetImplicitValence(). The desired
invariant is that GetImplicitValence should never return a value less
than GetValence, i.e. implicit hydrogens or radicals may be present,
but the explicit bonds can never be ignored.
Index: src/atom.cpp
===
*** src/atom.cpp(revision 4680)
--- src/atom.cpp(working copy)
*** namespace OpenBabel
*** 501,508
unsigned int OBAtom::GetImplicitValence() const
{
//Special case for [H] to avoid infite loop: SMARTS Match() <->
AssignSpinMultiplicity()
! if(GetAtomicNum() == 1)
! return GetFormalCharge() ? 0 : 1;
OBMol *mol = (OBMol*)((OBAtom*)this)->GetParent();
if (mol && !mol->HasImplicitValencePerceived())
atomtyper.AssignImplicitValence(*((OBMol*)((OBAtom*)this)->GetParent()));
--- 501,512
unsigned int OBAtom::GetImplicitValence() const
{
//Special case for [H] to avoid infite loop: SMARTS Match() <->
AssignSpinMultiplicity()
! if(GetAtomicNum() == 1) {
! unsigned int val = GetValence();
! if (val == 0 && GetFormalCharge() == 0 && GetSpinMultiplicity() == 0)
! return 1;
! return val;
! }
OBMol *mol = (OBMol*)((OBAtom*)this)->GetParent();
if (mol && !mol->HasImplicitValencePerceived())
atomtyper.AssignImplicitValence(*((OBMol*)((OBAtom*)this)->GetParent()));
Thanks in advance,
Roger
--
Roger Sayle, Ph.D.
CEO and founder
NextMove Software Ltd
Cambridge, UK
--
Cloud Services Checklist: Pricing and Packaging Optimization
This white paper is intended to serve as a reference, checklist and point of
discussion for anyone considering optimizing the pricing and packaging model
of a cloud services business. Read Now!
http://www.accelacomm.com/jaw/sfnl/114/51491232/___
OpenBabel-Devel mailing list
OpenBabel-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Re: [OpenBabel-Devel] Simplying (and fixing) bindings
I'm afraid, it's not possible to ask disutils the location of the site-packages folder (believe me, I have tried, googled, everything). As a result, we have no choice. In any case, this simplifies everything and I wouldn't change it back now. The user just needs to add the lib directory to PYTHONPATH, RUBYPATH, CLASSPATH, PERL5LIB, etc. For distributions, it's not a problem. They can just copy the relevant files to the appropriate location as part of making the package. In fact, it'll work much better now that DESTDIR is supported out-of-the-box. - Noel On 7 December 2011 17:14, My Th wrote: > Hi! > > Please teach CMake to install bindings in standard locations as > distutils is doing. That should help distributions also. > > > Reinis -- Cloud Services Checklist: Pricing and Packaging Optimization This white paper is intended to serve as a reference, checklist and point of discussion for anyone considering optimizing the pricing and packaging model of a cloud services business. Read Now! http://www.accelacomm.com/jaw/sfnl/114/51491232/ ___ OpenBabel-Devel mailing list OpenBabel-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Re: [OpenBabel-Devel] Simplying (and fixing) bindings
Hi! Please teach CMake to install bindings in standard locations as distutils is doing. That should help distributions also. Reinis -- Cloud Services Checklist: Pricing and Packaging Optimization This white paper is intended to serve as a reference, checklist and point of discussion for anyone considering optimizing the pricing and packaging model of a cloud services business. Read Now! http://www.accelacomm.com/jaw/sfnl/114/51491232/ ___ OpenBabel-Devel mailing list OpenBabel-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Re: [OpenBabel-Devel] Simplying (and fixing) bindings
Hi Noel, On Tue, Dec 6, 2011 at 3:25 PM, Noel O'Boyle wrote: > Hi there, > > I eat my words, Marcus - you were right. We should have been using the > CMake build system for the bindings, rather than using distutils for > Python, something else for Ruby, and so forth. There were various > reasons we (or I rather) took that decision, but they are overwhelmed > now by the maintainence problems we've had as people move to new > systems (64-bit Macs especially). It is always nice to hear I was right ;-) We have built quite a bit of logic into CMake to handle various platforms and architectures. I was largely concerned about the disconnect between the distutils (and friends) and the CMake understanding of the place you are installing to. I am traveling right now, but will happily take a look at the changes. If you hit any bugs please let me know, I would like to try and get any issues fixed up in CMake too. Marcus -- Cloud Services Checklist: Pricing and Packaging Optimization This white paper is intended to serve as a reference, checklist and point of discussion for anyone considering optimizing the pricing and packaging model of a cloud services business. Read Now! http://www.accelacomm.com/jaw/sfnl/114/51491232/ ___ OpenBabel-Devel mailing list OpenBabel-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openbabel-devel
[OpenBabel-Devel] [ openbabel-Bugs-3453452 ] CIF format reader doesn't handle alternate origin choices
Bugs item #3453452, was opened at 2011-12-07 02:54 Message generated for change (Tracker Item Submitted) made by coudert You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=428740&aid=3453452&group_id=40728 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: Library Functions Group: 2.3.x Status: Open Resolution: None Priority: 5 Private: No Submitted By: François-Xavier Coudert (coudert) Assigned to: Nobody/Anonymous (nobody) Summary: CIF format reader doesn't handle alternate origin choices Initial Comment: The attached CIF file is read incorrectly by OpenBabel. It has space group "P 4/n m m" and IT_coordinate_system_code == 2. If you convert it to PDB with --fillUC, the structure obtained is not correct, as the alternate origin has not been taken into account (I've checked the openbabel CIF reading code, and it doesn't try to handle this element). How to reproduce the issue: 1. Run "babel --fillUC strict AWW.cif AWW.pdb" 2. Compare the AWW.pdb generated with a correct AWW.pdb file, as converted by CrystalMaker for example (all three files are attached) -- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=428740&aid=3453452&group_id=40728 -- Cloud Services Checklist: Pricing and Packaging Optimization This white paper is intended to serve as a reference, checklist and point of discussion for anyone considering optimizing the pricing and packaging model of a cloud services business. Read Now! http://www.accelacomm.com/jaw/sfnl/114/51491232/ ___ OpenBabel-Devel mailing list OpenBabel-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openbabel-devel
