Hi, Noel! Fair enough. Thanks for fixing this.
As for the indexing issue (patch 3196280, 3202124) - as explained in patch 3202124, the issue was that OBMol::GetInternalCoord() returned a vector of N or N+1 elements depending on whether OBMol::SetInternalCoord() has been called before or not. In most of places the code prepends the internal coordinate vector with NULL and then the existing indexing works. But this is not documented and if there is not a strong reason to add this extra element, then I suggest to not do that, because when using OB Python bindings it was not intuitive at all that there has to be an extra NULL element at the beginning of internal coordinate vector. This changes the behavior of OB a bit, so I'm not sure if these patches (in the current form) are appropriate for the bugfix release. Reinis Sv, 2011-05-29 20:33 +0100, Noel O'Boyle rakstīja: > This bug is now fixed in r4494. Thanks for reporting. > > Your patch relies on implementation details of the iterators which may > change in future, so instead created a temporary vector in the first > loop, and then deleted the atoms in the vector. > > - Noel > > On 27 May 2011 13:28, My Th <rei4...@gmail.com> wrote: > > I think what I'm doing in that patch is legal and well defined. > > > > DeleteAtom basically does this: > > _vatom.erase(_vatom.begin()+(atom->GetIdx()-1)); > > > > where _vatom is a vector of pointers to atoms. When the element of the > > vector is deleted then all the iterators and references to elements > > __after__ the position of the removed element become invalid. So if I > > decrement the iterator back to the last unchanged position and then > > increment it again it should point to the next element after the removed > > one, and it does. > > > > > > Reinis > > > > Pk, 2011-05-27 12:49 +0100, Noel O'Boyle rakstīja: > >> Regarding Patch 3196285, AFAIK, all of our iterators are undefined > >> when you call DeleteAtom (this is a C++ idiom). The recommended way of > >> doing this is to append the atom to be deleted to a vector, and then > >> delete all members of the vector once the loop is over. > >> > >> Anyone else? > >> > >> - Noel > >> > >> On 27 May 2011 12:14, My Th <rei4...@gmail.com> wrote: > >> > Hi! > >> > > >> > A while ago I posted these patches in the tracker: > >> > https://sourceforge.net/tracker/?func=detail&aid=3196280&group_id=40728&atid=428742 > >> > https://sourceforge.net/tracker/?func=detail&aid=3196285&group_id=40728&atid=428742 > >> > > >> > The first one fixes indexing in obutil.cpp::InternalToCartesian(), the > >> > second one fixes removal of dummy atoms in the same function. > >> > > >> > Before the code for removing dummy atoms never was executed and dummy > >> > atoms were not removed. These patches fixes that, so the dummy atoms are > >> > removed when the function obutil.cpp::InternalToCartesian() is called. > >> > > >> > These patches still are not committed. Is there some technical reason > >> > for that? Could they be applied before the next point release of OB? > >> > > >> > > >> > Regards, > >> > Reinis > >> > > >> > > >> > ------------------------------------------------------------------------------ > >> > vRanger cuts backup time in half-while increasing security. > >> > With the market-leading solution for virtual backup and recovery, > >> > you get blazing-fast, flexible, and affordable data protection. > >> > Download your free trial now. > >> > http://p.sf.net/sfu/quest-d2dcopy1 > >> > _______________________________________________ > >> > OpenBabel-Devel mailing list > >> > OpenBabel-Devel@lists.sourceforge.net > >> > https://lists.sourceforge.net/lists/listinfo/openbabel-devel > >> > > > > > > > ------------------------------------------------------------------------------ Simplify data backup and recovery for your virtual environment with vRanger. Installation's a snap, and flexible recovery options mean your data is safe, secure and there when you need it. Data protection magic? Nope - It's vRanger. Get your free trial download today. http://p.sf.net/sfu/quest-sfdev2dev _______________________________________________ OpenBabel-Devel mailing list OpenBabel-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openbabel-devel
