Move cvm::atom member data to synchronized vectors within cvm::atom_group

[giacomofiorin]

This issue can be broken up in multiple steps:

• Transform the atom_group class into a container for vectors of shape xxxx..., yyyy... and zzzz... for positions, velocities and total forces.
• Change the atom class into an iterator of the atom_group vectors, which implements for each property: (a) a property() accessor returning a copy of the corresponding property as a 3-vector, (b) a set_property() function that takes a 3-vector and writes it to corresponding location in the atom_group vector, and (c) an incr_property() function.
• Add pointers inside atom_group to vectors containing the gradients and fit gradients of each CVC and of each of its components (e.g. x through z for vector-valued CVCs). For CVCs that are affected by the fit, gradients and fit gradients should be requested at the same time to the atom group. For those that don't.
• Add laboratory-frame versions of the above if useful.
• Implement exceptions to the above for those CVCs like distancePairs where the number of components is prohibitive.
• Collect pointers to all of the above (positions, total forces, gradients, fit gradients) into a single class (nested in the atom_group namespace), together with a unique integer allowing a CVC to search the corresponding object in the atom group. This should allow quick access and garbage collection when a CVC is deleted but not the atom group.

[jhenin]

Another route would be moving gradients to the cvc objects - at least that seems more intuitive.

[giacomofiorin]

Yeah, but then the cvc objects have to deal with changes in frame of reference, fit gradients, and such. With no solution giving a perfect division of labor, I prefer having a lightweight CVC class and a heavy atom group class, instead of two classes that are both heavy.

[jhenin]

This means that at some point, when a CVC applies a force to an atom group, the atom group will have to look up among its stored gradients which ones are those of that particular CVC. My impression is that it would be simpler for the CVC to pass a reference to its own gradients, together with the force.

One way or another, a reference to the gradients will be needed, but I suppose it doesn't make a difference which object they are actually stored in... Except, say, when the CVC is deleted, in which case its gradients should be deleted as well. If the gradients were stored in the CVC, the atom group wouldn't need to know that a CVC has come and gone. It would just receive forces, or not. So to me the current setup seems like the atom group is doing someone else's work for them. Of course that is natural in the current situation where the atom group belongs to the CVC anyway, but not if that changes.

[giacomofiorin]

The atom group can easily store pointers to the arrays of gradients, and set a pointers to NULL when the corresponding CVC are deleted. To store atomic data (positions vs gradients) in two different classes sounds to me more complex, actually.

…

On Tue, Mar 14, 2017 at 4:25 PM, Jérôme Hénin ***@***.***> wrote: This means that at some point, when a CVC applies a force to an atom group, the atom group will have to look up among its stored gradients which ones are those of that particular CVC. My impression is that it would be simpler for the CVC to pass a reference to its own gradients, together with the force. One way or another, a reference to the gradients will be needed, but I suppose it doesn't make a difference which object they are actually stored in... Except, say, when the CVC is deleted, in which case its gradients should be deleted as well. If the gradients were stored in the CVC, the atom group wouldn't need to know that a CVC has come and gone. It would just receive forces, or not. So to me the current setup seems like the atom group is doing someone else's work for them. Of course that is natural in the current situation where the atom group belongs to the CVC anyway, but not if that changes. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#61 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AETZWWg0QClN_VRZ_ROCO_2RZbHmI68Zks5rlvefgaJpZM4IxYGN> .

-- Giacomo Fiorin Associate Professor of Research, Temple University, Philadelphia, PA Contractor, National Institutes of Health, Bethesda, MD http://goo.gl/Q3TBQU https://github.com/giacomofiorin

[HanatoK]

Yeah, but then the cvc objects have to deal with changes in frame of reference, fit gradients, and such. With no solution giving a perfect division of labor, I prefer having a lightweight CVC class and a heavy atom group class, instead of two classes that are both heavy.

Here's my thoughts. I think if the nesting CVs are allowed, then the gradients can be put into CVC class while still being lightweight. Let's say if there is a class derived from CVC named AlignedAtoms, then you can put all fitting algorithm into it. Apart from the values and gradients of atomic coordinates, AlignedAtoms also provides interfaces to query atomic indexes and names. It just like a moving frame of virtual atoms. Since CVCs can be nested, so in the end we can have some configuration like:

AlignedAtoms {
    name        aligned_reference
    indexGroup  reference
    atoms {
        centerReference    on
        rotateReference    on
        fittingGroup {
            indexGroup  protein
        }
        refpositionsfile  ./complex_largeBox.xyz
    }
}

AlignedAtoms {
    name        aligned_ligand
    indexGroup  ligand
    atoms {
        centerReference    on
        rotateReference    on
        fittingGroup {
            indexGroup  protein
        }
        refpositionsfile  ./complex_largeBox.xyz
    }
}

colvar {
    name polarPhi
    customFunction atan2(i3, i1) * 180 / 3.1415926
    period  360
    wrapAround 0.0
    distanceDir {
        name  i
        group1 {
            virtualAtoms aligned_reference
        }
        group2 {
            virtualAtoms aligned_ligand
        }
    }
}

In this way, you can move the gradients into CVCs (such as polarPhi above), and don't need to handle the fitting gradients in it. The fitting gradients can be handled by AlignedAtoms. When applying forces the chain rule should be OK.

[jhenin]

A related point: I've noted that in practice, fitting is nearly often done per-CVC and not per atom group. I can't think of an example where different atom groups are fitted in a different way within a CVC, and intuitively it doesn't make much sense to me.
So maybe the task could be offloaded to the CVC, avoiding redundant calculations.

[giacomofiorin]

@HanatoK Whichever way you look at it, gradients and fit gradients are specific to the combination of CVC and atom group, so storage for those will be shared by both, requiring changes to the existing layout of both classes. It would be good not to reinvent the wheel, so at the moment I'm trying to see whether we can finally use shared_ptr or something similar, and the CVC gradient computation is not something that one could #ifdef out of VMD.

@jhenin Again along those lines, a fit is determined by the CVC-group combination, so storage in either class would make sense to me. But the most immediate case of redundant calculations that you could think of are the path CVs, where you fit to a different reference structure each time and so the fit is different for each: I don't see how can you save work by moving the fitting around, as most of the terms are not redundant after all. Am I missing something?

[jhenin]

I agree that gradients and fit gradients are specific to the combination of CVC and atom group. But while an atom group could potentially be shared and re-used between different CVCs, a single CVC cannot be "shared/re-used between different atom groups". So to me the only unbreakable link is between gradient and CVC.

About redundant fits, I meant fitting several atom groups independently within the same CVC. The most common case of this happens with distanceVec and distanceZ.

[giacomofiorin]

I think it's more than that. With distanceVec or distanceZ it's not just the fitting part (which isn't even there all the time) but the entire function that is computed redundantly. Hence #232 no?

[jhenin]

There are cases for distanceZ when several projections are needed, yes - that would be good to address as well. For distanceVec, I can't think of a case. Here I was only addressing the case of fitting both groups the same way.

PS: if this is going to be an architecture change, it might as well address all these issues at once if possible.

[giacomofiorin]

I can't think of a way that an architecture change, no matter how limited, wouldn't be involved at some point. We're discussing redundancies between different groups of the same CVC, different CVCs of the same colvar and different colvars altogether. That's three levels of hierarchy. Issues like this one don't stay open for 5+ years based on procrastination alone.

So any discussion is of course useful/welcome to see all sides of it. More automated (unit) tests would also help, even if those cover code that is very stable by now, but would need to be changed for performance. Admittedly that's less visible work, but no less important.