Update() and updatePrimitive() not called at molecule loading

Development
#1

Hi,

today I committed the geometric-information stuff in the Molecule class. As
you told me, Geoff, I added calls to computeGeometricInfo() in the following
two functions:

Molecule::updatePrimitive()
Molecule::update()

and I added qDebug()'s to check that they were actually being called, and I
was surprised to see that these functions don’t seem to be called at all at
molecule loading! Am I missing something? However, adding an atom results in
a call to Molecule::updatePrimitive().

Cheers,
Benoit

#2

Woops. Replace “Geoff” with “Donald” in my previous e-mail. Sorry.

On Wednesday 14 March 2007 13:06:11 Benoît Jacob wrote:

Hi,

today I committed the geometric-information stuff in the Molecule class. As
you told me, Geoff, I added calls to computeGeometricInfo() in the
following two functions:

Molecule::updatePrimitive()
Molecule::update()

and I added qDebug()'s to check that they were actually being called, and I
was surprised to see that these functions don’t seem to be called at all at
molecule loading! Am I missing something? However, adding an atom results
in a call to Molecule::updatePrimitive().

Cheers,
Benoit

#3

On Mar 14, 2007, at 1:41 PM, Donald Ephraim Curtis wrote:

function in OB that you can pas a new OBAtom* and then the OBMol adds
that atom.

What? See OBMol::AddAtom()
http://openbabel.sourceforge.net/dev-api/
classOpenBabel_1_1OBMol.shtml#a53209ef174b454eb99e269ef59ebbe0

What’s wrong with that?

Cheers,
-Geoff

#4

right, because this is called when things change. The adding atom ting
is different because the way that OB works. Ideally there should be a
function in OB that you can pas a new OBAtom* and then the OBMol adds
that atom. Right now what happens is that the atom gets duplicated so
you must lt OBMol give you a new OBMol.

The thing is, what i was talking about was with caching that value.
There should be a call Molecule::geometricInfo which can check if the
geometric info has already been computed, if not, compute it. I guess
those two calls only need to invalidate that data.

-Donald

(Wed, Mar 14, 2007 at 01:06:11PM +0100) Benoît Jacob jacob@math.jussieu.fr:

Hi,

today I committed the geometric-information stuff in the Molecule class. As
you told me, Geoff, I added calls to computeGeometricInfo() in the following
two functions:

Molecule::updatePrimitive()
Molecule::update()

and I added qDebug()'s to check that they were actually being called, and I
was surprised to see that these functions don’t seem to be called at all at
molecule loading! Am I missing something? However, adding an atom results in
a call to Molecule::updatePrimitive().

Cheers,
Benoit

Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net’s Techsay panel and you’ll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Avogadro-devel mailing list
Avogadro-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/avogadro-devel

#5

On Mar 14, 2007, at 2:36 PM, Donald Ephraim Curtis wrote:

This isn’t bad, it just means that at the point i add my atom to the
molecule it doesn’t have the coordinates that i want (ie. NewAtom()
sets
to 0 coordinates or something).

Yes, it really creates a new atom, just like “new OBAtom” would.

I guess what I’m saying is that I don’t see a problem. But if you do,
please let me know so we can fix it before 2.1 is released.

Thanks,
-Geoff

#6

On Wednesday 14 March 2007 18:41:56 Donald Ephraim Curtis wrote:

The thing is, what i was talking about was with caching that value.
There should be a call Molecule::geometricInfo which can check if the
geometric info has already been computed, if not, compute it. I guess
those two calls only need to invalidate that data.

aah, I hadn’t understood that. I think that’s a better idea than mine, so I’ll
do it, and it’s fortunate that Qt provides the functionality to do that
caching in a thread-safe.

However, there remains a problem: as you say we still need calls to invalidate
the cached data. That leaves that question open: where to put these calls, so
that the geometric info gets computed at molecule loading?

Another thing to keep in mind is that molecule loading is a bit special as all
the N atoms get added one after the other, so if computeGeometricInfo has
O(n) complexity, and gets called for each of the N atoms, then the whole
process has O(N^2) complexity. For large compounds with thousands of atoms,
this would become noticeable. Hence it would be much better to make it so
that at molecule loading, computeGeometricInfo() only gets called once, after
the last atom has been added. I can think of one way to do that: add an
optional argument to CreateAtom(),

Molecule::CreateAtom( bool invalidateGeometricInfo = true )

and at molecule loading, call CreateAtom( false ).

I hope that makes sense,

Benoit

#7

aaaaaaaah I’m so stupid. I hadn’t understood that the benefit of your
caching-invalidating approach is that we don’t need to compute the geometric
info before we use it, so we don’t need to compute it at molecule loading. I
guess that’s what’s called “lazy evaluation”. OK, so please ignore my
previous mail and sorry for the noise. I’ll do it tomorrow.

Benoit

On Wednesday 14 March 2007 19:05:29 Benoît Jacob wrote:

On Wednesday 14 March 2007 18:41:56 Donald Ephraim Curtis wrote:

The thing is, what i was talking about was with caching that value.
There should be a call Molecule::geometricInfo which can check if the
geometric info has already been computed, if not, compute it. I guess
those two calls only need to invalidate that data.

aah, I hadn’t understood that. I think that’s a better idea than mine, so
I’ll do it, and it’s fortunate that Qt provides the functionality to do
that caching in a thread-safe.

However, there remains a problem: as you say we still need calls to
invalidate the cached data. That leaves that question open: where to put
these calls, so that the geometric info gets computed at molecule loading?

Another thing to keep in mind is that molecule loading is a bit special as
all the N atoms get added one after the other, so if computeGeometricInfo
has O(n) complexity, and gets called for each of the N atoms, then the
whole process has O(N^2) complexity. For large compounds with thousands of
atoms, this would become noticeable. Hence it would be much better to make
it so that at molecule loading, computeGeometricInfo() only gets called
once, after the last atom has been added. I can think of one way to do
that: add an optional argument to CreateAtom(),

Molecule::CreateAtom( bool invalidateGeometricInfo = true )

and at molecule loading, call CreateAtom( false ).

I hope that makes sense,

Benoit

#8

Because, if i do this;

//----
OBMol *mol = new OBMol;
OBAtom *atom = new OBAtom;
atom->SetX(x);
atom->SetY(y);
atom->SetZ(z);
mol->AddAtom(*atom);
//—

Then after the last call, my atom pointer doesn’t point to the atom in
the molecule. OBMol duplicates my atom rather than adding it. So to
retain a copy of my pointer i have to do;

//----
OBMol *mol = new OBMol;
OBAtom *atom = mol->NewAtom()
…
//—

This isn’t bad, it just means that at the point i add my atom to the
molecule it doesn’t have the coordinates that i want (ie. NewAtom() sets
to 0 coordinates or something).

-Donald

(Wed, Mar 14, 2007 at 01:08:23PM -0400) Geoffrey Hutchison geoff.hutchison@gmail.com:

On Mar 14, 2007, at 1:41 PM, Donald Ephraim Curtis wrote:

function in OB that you can pas a new OBAtom* and then the OBMol adds
that atom.

What? See OBMol::AddAtom()
http://openbabel.sourceforge.net/dev-api/
classOpenBabel_1_1OBMol.shtml#a53209ef174b454eb99e269ef59ebbe0

What’s wrong with that?

Cheers,
-Geoff

#9

It’s not something we can fix for 2.1. It’s just the way the OBMol
works. I’ll do my best to explain what is happening.

In Avogadro we subclassed OBMol and then implemented the virtual
Create* function so that we can do callbacks when things are created.
But lets take for instance CreateAtom.

At the time CreateAtom is called we know nothing about the atom (no
coordinates, no element type), just that it was created. So I can add
this atom to the GLWidget view, but it will not render it in the desired
location because again, at the time of calling CreateAtom we don’t know
the coordinates of the atom. This means that in the “draw” tool we have
to do the atom creation and then once we have actually set the
coordinates, we have to make another call (which i designed as
::update()) which says, “hey, coordinates changed for this atom better
redraw”. And the draw tool needs to keep pointers to atoms which it’s
created at least while it’s running.

This is avoided when we “load” for the following reason: when the
GLWidget::setMolecule() is called the GLWidget gets all the
Atoms/Bonds/Residues that already exist and add it to the view, then
connects to the Molecules signals “watching” for changes after it’s been
initially loaded.

I just think that it would be more convenient in the future (3.0) to not
require the users of libopenbabel to have to use OBMol::NewAtom to add
new atoms. ie. I should be able to create a new Atom pointer, setup
the coordinates, THEN add it to the molecule with the stipulation that
my "new"ly allocated atom is added, not a copy of my atom. Because
after i add a new atom i want to keep my pointer to the same atom.

I think this has never been a big issue because normally Molecules are
created and destroyed, not really modified all that much. So using
indexes was a fine approach, but this isn’t the best approach for us and
having to rely on indexes is much more error prone in my opinion.

(Wed, Mar 14, 2007 at 01:49:34PM -0400) Geoffrey Hutchison geoff.hutchison@gmail.com:

On Mar 14, 2007, at 2:36 PM, Donald Ephraim Curtis wrote:

This isn’t bad, it just means that at the point i add my atom to the
molecule it doesn’t have the coordinates that i want (ie. NewAtom()
sets
to 0 coordinates or something).

Yes, it really creates a new atom, just like “new OBAtom” would.

I guess what I’m saying is that I don’t see a problem. But if you do,
please let me know so we can fix it before 2.1 is released.

Thanks,
-Geoff
(Wed, Mar 14, 2007 at 01:49:34PM -0400) Geoffrey Hutchison geoff.hutchison@gmail.com:

On Mar 14, 2007, at 2:36 PM, Donald Ephraim Curtis wrote:

This isn’t bad, it just means that at the point i add my atom to the
molecule it doesn’t have the coordinates that i want (ie. NewAtom()
sets
to 0 coordinates or something).

Yes, it really creates a new atom, just like “new OBAtom” would.

I guess what I’m saying is that I don’t see a problem. But if you do,
please let me know so we can fix it before 2.1 is released.

Thanks,
-Geoff