HI Paolo,
On Thu, May 17, 2012 at 11:16 AM, Paolo Tosco <paolo.to...@unito.it> wrote:
> I may be wrong here, but my impression is that both OBMol and OBAtom =
> operators work fine, the problem is that formal charges are zeroed after
> the call to Clear() in the OBMol = operator, as can ve verified printing
> the results of GetFormalCharge() before and after Clear().
>
I found the opposite: Clear() had no effect on the formal charge of the
source molecule. The call to Clear() only affects the destination
molecule. (In fact, it's hard to see how Clear() could affect the source
molecule since it's a method on the destination object.)
When I put print statements in the OBAtom::GetType() method, it was clear
that the charge was OK on the very first entry (i.e. when the first atom's
type was requested) and erased by the time the method returned. It doesn't
just erase the one atom ... it gets them all when the first atom's type is
requested. That's why the first atom's charge is OK but subsequent atoms
have zero charge. If the Clear() method was the problem, then even the
first atom's charge would be zero.
Craig
> Therefore what OBAtom::Duplicate duplicates are actually zeros, except for
> the +2 charge that the atom typer is able to assign. Apparently, the atom
> typer can assign some formal charges (e.g., in your example the +2 charge
> of one of the Fe ions), but some of them need to be read from user input,
> since they cannot be univocally determined.
> I had this problem to assign MMFF94 atom types to Cu2+and Cu+. There is no
> way the atom typer can understand if you have Cu(I) or Cu(II), so correct
> MMFF94 requires formal charge input by the user, such as the one supplied
> in SDF files.
> It could be that I'm completely wrong, since I don't have a comprehensive
> picture of the way OB works, I'm just telling you about my experience, so
> maybe someone can find a better patch than mine (which works, though :-)
>
> Cheers,
> p.
>
>
> On 05/17/2012 08:00 PM, Craig James wrote:
>
> I have narrowed down the "lost charge" problem to atomtyper.cpp.
> OBMol::operator= calls OBAtom::operator=, which calls the atom typer to
> figure out the type of the *source* atom. During that process, the atom
> typer erases the charge on the source atoms.
>
> I'm still digging, but I'm way beyond my area of familiarity with the OB
> code. Paolo's patch is a workaround, but this is a pretty serious bug in
> the OBAtom::operator=.
>
> Craig
>
>
> On Thu, May 17, 2012 at 9:38 AM, Craig James <cja...@emolecules.com>wrote:
>
>>
>>
>> On Thu, May 17, 2012 at 9:04 AM, Chris Morley <c.mor...@gaseq.co.uk>wrote:
>>
>>> Craig's failing example works fine for me under Windows with both 2.3.1
>>> and the development code. As he suggested, this inconsistency suggests
>>> that there is something not being initialized properly. The logic around
>>> the flag OB_TCHARGE_MOL is a bit tricky and so is a candidate, but I
>>> cannot see anything wrong. Paolo's patch is in the same area in the
>>> OBMol assignment operator (which is used in SMILES output) but as far as
>>> I can see it just duplicates the copying of atom formal charges which is
>>> already done in the OBAtom::Duplicate(). I think it would be better to
>>> identify the fault before applying the patch. But it is tricky finding a
>>> bug if you cannot reproduce it. Can anyone help?
>>>
>>
>>
>> Excellent point. It turns out the SMILES format copies the molecule
>> before generating the SMILES. The "=" operator seems to be the culprit. I
>> modified smilesformat.cpp as follows:
>>
>>
>> bool SMIBaseFormat::WriteMolecule(OBBase* pOb,OBConversion* pConv)
>> {
>> //cout << "SMIBaseFromat::WriteMolecule()" << endl;
>> OBMol* pmol = dynamic_cast<OBMol*>(pOb);
>>
>> FOR_ATOMS_OF_MOL(atom, pmol) {
>> fprintf(stderr, "smilesformat: atom %d: formal charge: %d, spin
>> mult: %d\n",
>> atom->GetIdx(), atom->GetFormalCharge(),
>> atom->GetSpinMultiplicity());
>> }
>>
>> // Define some references so we can use the old parameter names
>> ostream &ofs = *pConv->GetOutStream();
>> OBMol mol = *pmol;
>>
>> FOR_ATOMS_OF_MOL(atom, mol) {
>> fprintf(stderr, "smilesformat: atom %d: formal charge: %d, spin
>> mult: %d\n",
>> atom->GetIdx(), atom->GetFormalCharge(),
>> atom->GetSpinMultiplicity());
>> }
>>
>> Here's what it prints. Simply copying the molecule with the "=" operator
>> loses the charges.
>>
>> smilesformat: atom 1: formal charge: 2, spin mult: 0
>> smilesformat: atom 2: formal charge: -2, spin mult: 0
>> smilesformat: atom 3: formal charge: 2, spin mult: 0
>> smilesformat: atom 4: formal charge: -2, spin mult: 0
>> smilesformat: atom 1: formal charge: 2, spin mult: 0
>> smilesformat: atom 2: formal charge: 0, spin mult: 0
>> smilesformat: atom 3: formal charge: 0, spin mult: 0
>> smilesformat: atom 4: formal charge: 0, spin mult: 0
>>
>> I'll keep digging... but if anyone recalls making a change that's related
>> to this, we could zero in faster.
>>
>> Thanks,
>> Craig
>>
>>
>>>
>>> Chris
>>>
>>> On 17/05/2012 16:28, Craig James wrote:
>>> > Hi Paolo,
>>> >
>>> > Thanks very much for the patch. Unfortunately, the OBForceField
>>> > functions that you patched are never called at all during the
>>> > SDF-to-SMILES conversion process, so the patch doesn't resolve my
>>> > issue. OBForceField only seems to be called if I use the --gen3D
>>> option.
>>> >
>>> > Best regards,
>>> > Craig
>>> >
>>> > On Thu, May 17, 2012 at 3:03 AM, Paolo Tosco <paolo.to...@unito.it
>>> > <mailto:paolo.to...@unito.it>> wrote:
>>> >
>>> > Dear Craig,
>>> >
>>> > I have faced the identical problem with MMFF94, and I have
>>> > previously submitted a similar patch which has already been applied
>>> > to SVN OpenBabel. However, now I realize that my fix had only
>>> solved
>>> > my own problem, but not a more general problem connected with
>>> formal
>>> > charges.
>>> > So I have attached a much better patch, which reverts my previous
>>> > one, and instead applies it on the OBMol copy constructor, so that
>>> > it becomes of general use.
>>> >
>>> > To make a long story short, if you patch the current OpenBabel SVN
>>> > version with the attached patch your issue will be solved.
>>> >
>>> > From the OpenBabel root folder:
>>> >
>>> > patch -p0 < patch_17-05-2012.patch
>>> >
>>> > Dear Geoff, would you please apply it also to the SVN trunk so that
>>> > also other OpenBabel users may benefit from that? It also includes
>>> a
>>> > small patch on "A T O M T Y P E S" logging that I had left behind
>>> > previously.
>>> >
>>> > Best regards,
>>> > Paolo
>>> >
>>> >
>>> > On 05/17/2012 01:44 AM, Craig James wrote:
>>> >> Here's another problem in 2.3.x that's not in 2.2.x. I realize
>>> >> these are bogus molecules, but it illustrates a serious problem.
>>> >>
>>> >> $ echo "[Fe+2][Fe-2].[Fe+2].[Fe-2]" | babel -i smi -o can
>>> >> [Fe-2][Fe+2].[Fe-2].[Fe+2]
>>> >>
>>> >> Everything looks good -- charges are maintained. Now go through
>>> >> the SDF format:
>>> >>
>>> >> $ echo "[Fe+2][Fe-2].[Fe+2].[Fe-2]" | babel -i smi -o sdf |
>>> >> babel -i sdf -o can
>>> >> [Fe+2][Fe].[Fe].[Fe]
>>> >>
>>> >> Not so good -- charges are dropped. Where is the problem? It
>>> >> turns out the SDF is correct:
>>> >>
>>> >> $ echo "[Fe+2][Fe-2].[Fe+2].[Fe-2]" | babel -i smi -o sdf
>>> >>
>>> >> OpenBabel05161216052D
>>> >>
>>> >> 4 1 0 0 0 0 0 0 0 0999 V2000
>>> >> 0.0000 0.0000 0.0000 Fe 0 2 0 0 0 0 0 0 0 0
>>> 0 0
>>> >> 0.0000 0.0000 0.0000 Fe 0 6 0 0 0 0 0 0 0 0
>>> 0 0
>>> >> 0.0000 0.0000 0.0000 Fe 0 2 0 0 0 15 0 0 0 0
>>> 0 0
>>> >> 0.0000 0.0000 0.0000 Fe 0 6 0 0 0 15 0 0 0 0
>>> 0 0
>>> >> 1 2 1 0 0 0 0
>>> >> M CHG 4 1 2 2 -2 3 2 4 -2
>>> >> M END
>>> >> $$$$
>>> >>
>>> >> The charges are all properly marked in the atom block and in the M
>>> >> CHG line. And the "15" in atoms 3 and 4 means "zero valence"
>>> >> which is right. Yet the SMILES is wrong.
>>> >>
>>> >> It turns out this same thing happens if you use
>>> >> atom->SetFormalCharge(). It sometimes works and sometimes
>>> doesn't.
>>> >>
>>> >> Note that it works correctly in OpenBabel 2.2.x:
>>> >>
>>> >> $ echo "[Fe+2][Fe-2].[Fe+2].[Fe-2]" | babel -i smi -o sdf |
>>> >> babel -i sdf -o can
>>> >> [Fe-2].[Fe-2][Fe+2].[Fe+2]
>>> >>
>>> >> Anyone have any idea what changed, either in the OpenBabel
>>> >> internals or the SMILES generator, that would affect the way
>>> >> charges are interpreted?
>>> >>
>>> >> Craig
>>>
>>>
>>> ------------------------------------------------------------------------------
>>> Live Security Virtual Conference
>>> Exclusive live event will cover all the ways today's security and
>>> threat landscape has changed and how IT managers can respond. Discussions
>>> will include endpoint security, mobile security and the latest in malware
>>> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
>>> _______________________________________________
>>> OpenBabel-discuss mailing list
>>> OpenBabel-discuss@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/openbabel-discuss
>>>
>>
>>
>
>
> ------------------------------------------------------------------------------
> Live Security Virtual Conference
> Exclusive live event will cover all the ways today's security and
> threat landscape has changed and how IT managers can respond. Discussions
> will include endpoint security, mobile security and the latest in malware
> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
>
>
>
> _______________________________________________
> OpenBabel-discuss mailing
> listOpenBabel-discuss@lists.sourceforge.nethttps://lists.sourceforge.net/lists/listinfo/openbabel-discuss
>
>
>
> --
> ==========================================================
> Paolo Tosco, Ph.D. Phone: +39 011 6707680
> Department of Drug Science Fax: +39 011 6707687
> and Technology Mob: +39 348 5537206
> Via Pietro Giuria, 9
> 10125 Torino, Italy
> http://open3dalign.org
> E-mail: paolo.to...@unito.it http://open3dqsar.org
> ==========================================================
>
>
>
> ------------------------------------------------------------------------------
> Live Security Virtual Conference
> Exclusive live event will cover all the ways today's security and
> threat landscape has changed and how IT managers can respond. Discussions
> will include endpoint security, mobile security and the latest in malware
> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
> _______________________________________________
> OpenBabel-discuss mailing list
> OpenBabel-discuss@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openbabel-discuss
>
>
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
OpenBabel-discuss mailing list
OpenBabel-discuss@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openbabel-discuss
