Start of transfer of functionality from hack-the-tree to maser #933
Conversation
In hack-the-tree you can pass vectors/matrices and grids through Values. This necessitates some changes to the Value class. In particular inputForce becomes a vector and similarly the double value becomes a vector called data. I avoid having a confusing vector called derivatives (which is not always the derivatives). If I have a scalar I store the value and the derivatives in the data vector. data thus has a length of 1 + number of derivatives. The first element is the value of the scalar. Notice that I also have functionality to set the value to be a constant that is calculated once and only once during the calculation
I added a few lines into the script here to test the code that allows you to get values from PLUMED and pass them back to the calling program. This means you can test this functionality even when you don't have python installed
…-tree This functionality introduces two new actions into plumed: PUT = an ActionWithValue that transfers some data from a pointer that is passed from the MD code to a PLMD::Value. GET = an ActionWithArgument that transfers data from a PLMD::Value to a pointer than is passed from the MD code Notice that you can also pass a pointer to the forces on a object that is passed to PUT. The forces from value are then added to the MD code's force pointer when you call the apply method. To demonstrate how this functionality works I have replaced all the stuff for passing extraCVs to PLUMED with PUT commands I have amm also using GET in place of the old DataFetchingObject class, which has now been deleted
This commit modifies the code so that cell vectors are passed through a PUT action. The virial is then passed as the forces that any forces on the input value are added on. To deal with the fact that the virial and cell should only be passed from one of the MD codes processes there is a special new class PbcAction that deals with passing the cell vectors. Methods from this class are also invoked when the box parameters are sought elswhere in the code. Notice that I have also added the DomainDecomposition class from hack-the-tree here. Much of the functionality in this class is not yet used. It also needs to be modified before it can be used here. I felt as we merge hack-the-tree it is easier to start out with a code that resembles the one in hack-the-tree.
The timestep is now passed to PLUMED using a PUT action. The timestep is thus stored in a value called timestep that can be accessed using the label timestep. Notice that changes to the object that collects the data from the MD code were required here because the timestep is passed directly (and not via a pointer). To resolve this there is a variable in DataPassingObject that can hold a double that is passed from the MD code.
kBT is now passed from the MD code into a PLMD::Value by passing the value to a PUT action. Whenever an action needs kBT you can call a method in action called getkBT. This reads in the keyword TEMP (if it is registered). If TEMP is present the value read for this flag is multiplied by Boltzmann's constant. If TEMP is not present then the kBT value that was passed from the MD code to PLUMED is returned.
This should resolve the problem that is cauased when you call setNatoms multiple times and it tries to create multiple DOMAIN_DECOMPOSITION objects
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #933 +/- ##
==========================================
- Coverage 85.81% 85.54% -0.28%
==========================================
Files 600 610 +10
Lines 54717 55210 +493
==========================================
+ Hits 46958 47229 +271
- Misses 7759 7981 +222
☔ View full report in Codecov by Sentry. |
|
@GiovanniBussi I had another look at what I did here. I had to make changes to some of the cltools as you have to call setNoVirial before you call setNatoms. I checked though and I have also put some tests in to ensure that the code crashes if these two calls are in the wrong order. I think in regards to what we discussed in the meeting yesterday about the future of the interfaces we are thus fine. |
|
I guess setNoVirial is "rare" (I hope). Is the order of "setVirial" also fixed? Is there an easy to remember logic for this (e.g. box and virial both go before positions and forces)? |
|
@GiovanniBussi setVirial has to go after setNatoms but that is always going to happen as setVirial is not called during initialisation whereas setNatoms is called during initialisation. |
|
This makes totally sense |
The atomic positions, masses and charges are now passed from the MD code into PLMD::Values.
Five PLMD::Value objects are created to hold these quantities. These PLMD::Value objects are
all vectors with natoms elements. The ActionWithValue that owns each of thse PLMD::Values is
a PUT object. These PUT objects are created by a new action called DOMAIN_DECOMPOSITION. This
action is created when you call cmd("setNatoms"). However, this action could be created from the
MD code directly by doing cmd("readInputLine mdcode: DOMAIN_DECOMPOSITION"). This would then
give the flexibility to pass quantities other than the the positions, masses and charges to PLUMED.
N.B. The passing of energy between the MD code and PLUMED is broken by this commit. I will fix this
on the next commit.
|
I have a question about the Atoms class and the behaviour of ExtraCV. When positions, the cell and the energy are passed into PLUMED they are treated as if they are const. In other words, it is assumed that PLUMED will not change the data in the objects there are being passed to PLUMED. Meanwhile, forces and the viral are treated as non-const as PLUMED does have to change these objects. The one exception is ExtraCV, where the value of the CV that is passed to PLUMED is stored as a non-const object. This suggests there is some case where PLUMED changes the value of the ExtraCV that is passed into the code. Is this correct? What is the reason? And what would be a sensible way of testing this functionality as I don't think there is anything in the current tests where the value of the CV that is passed from the MD code to PLUMED is changed by PLUMED? |
|
I think that what matters for the API is how the pointer is converted. This
happens in MDAtoms.cpp line 99, where I think you can accept either a const
or non const pointer from the MD code, which should be correct.
Then within plumed if you store a double (and not a pointer) it’s tricky to
use a const, because then you cannot modify after initialization
I mean
Position is a pointer so it can be a pointer to const
Extra cv is a double so it should be non const
Still methods returning a reference to that double should return a const
reference (I didn’t double check now if this is true)
Let me know if this makes sense….
Il giorno dom 14 mag 2023 alle 11:48 Gareth Tribello <
***@***.***> ha scritto:
@GiovanniBussi <https://github.com/GiovanniBussi>
I have a question about the Atoms class and the behaviour of ExtraCV. When
positions, the cell and the energy are passed into PLUMED they are treated
as if they are const. In other words, it is assumed that PLUMED will not
change the data in the objects there are being passed to PLUMED.
Meanwhile, forces and the viral are treated as non-const as PLUMED does
have to change these objects.
The one exception is ExtraCV, where the value of the CV that is passed to
PLUMED is stored as a non-const object. This suggests there is some case
where PLUMED changes the value of the ExtraCV that is passed into the code.
Is this correct? What is the reason? And what would be a sensible way of
testing this functionality as I don't think there is anything in the
current tests where the value of the CV that is passed from the MD code to
PLUMED is changed by PLUMED?
—
Reply to this email directly, view it on GitHub
<#933 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABBNCXTBEZDKGCBTPQA274TXGCS5FANCNFSM6AAAAAAX2F6TWY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Sent from Gmail mobile
|
|
And yes I double checked now
Atoms::getExtraCV()
Returns a double by value, so the stored value cannot be modified in any way
On Sun, 14 May 2023 at 12:32, Giovanni Bussi ***@***.***> wrote:
I think that what matters for the API is how the pointer is converted.
This happens in MDAtoms.cpp line 99, where I think you can accept either a
const or non const pointer from the MD code, which should be correct.
Then within plumed if you store a double (and not a pointer) it’s tricky
to use a const, because then you cannot modify after initialization
I mean
Position is a pointer so it can be a pointer to const
Extra cv is a double so it should be non const
Still methods returning a reference to that double should return a const
reference (I didn’t double check now if this is true)
Let me know if this makes sense….
Il giorno dom 14 mag 2023 alle 11:48 Gareth Tribello <
***@***.***> ha scritto:
> @GiovanniBussi <https://github.com/GiovanniBussi>
>
> I have a question about the Atoms class and the behaviour of ExtraCV.
> When positions, the cell and the energy are passed into PLUMED they are
> treated as if they are const. In other words, it is assumed that PLUMED
> will not change the data in the objects there are being passed to PLUMED.
>
> Meanwhile, forces and the viral are treated as non-const as PLUMED does
> have to change these objects.
>
> The one exception is ExtraCV, where the value of the CV that is passed to
> PLUMED is stored as a non-const object. This suggests there is some case
> where PLUMED changes the value of the ExtraCV that is passed into the code.
> Is this correct? What is the reason? And what would be a sensible way of
> testing this functionality as I don't think there is anything in the
> current tests where the value of the CV that is passed from the MD code to
> PLUMED is changed by PLUMED?
>
> —
> Reply to this email directly, view it on GitHub
> <#933 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABBNCXTBEZDKGCBTPQA274TXGCS5FANCNFSM6AAAAAAX2F6TWY>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
Sent from Gmail mobile
--
Sent from Gmail mobile
|
The energy is now passed through a specialised PUT action. When apply is called for this PUT action the rescaleForces method of the position and forces PUT actions are called. This allows one to adjust the forces on the atoms in the correct way due to a force that acts on the value of the energy
If, when interpretting an atom list, ActionAtomistic encounters a group it now calls a method of the Group action with that label to get the relevant list of atoms. This means you no longer need to store all the groups in a map in the Atoms class.
|
I'm still not totally clear but I don't think my question was very clear. I'll give it another go. So when we pass the energy to PLUMED atoms does this: void Atoms::setEnergy(const TypesafePtr & p) {
plumed_massert( dataCanBeSet,"setEnergy must be called after setStep in MD code interface");
MD2double(p,md_energy);
md_energy*=MDUnits.getEnergy()/units.getEnergy();
energyHasBeenSet=true;
} Energy is thus saved in a variable that is local to PLUMED. The MD code's energy cannot be modified as we don't even save a pointer to the variable in the MD code that stores the energy. When we pass an ExtraCV, PLUMED does this: std::map<std::string,TypesafePtr> extraCV;
void Atoms::setExtraCV(const std::string &name,const TypesafePtr & p) {
mdatoms->setExtraCV(name,p);
}
void setExtraCV(const std::string &name,const TypesafePtr & p) override {
p.get<T>(); // just check type and discard pointer
extraCV[name]=p.copy();
}It strikes me that, in theory, we could do this instead: std::map<std::string,double> extraCV;
void Atoms::setExtraCV(const std::string &name,const TypesafePtr & p) {
double extracv; MD2double(p,extracv);
extraCV[name] = extracv;
} This would be more like what was done for energy. Furthermore, as with energy, if done this way you can pass the value of the extraCV directly instead of the pointer. A reason for not doing it this way is if you want PLUMED to be able to change the value of the extracv in the MD code but you don't want to do that so... why the difference from the way energy is set? |
|
I see the point. I think I did it for consistency with positions (even
though this is scalar so it was not necessary), and you correctly argue the
inconsistency with energy.
One important fact is that now this is crystallized in the gromacs patch,
see attachment: basically you can compute the bias multiple times while
doing the Monte Carlo on lambda, and setExtraCV is not called every time.
If we change it to pass by value it will break this.
Is there any specific reason (except for consistency with the energy) to do
so? Or could it be changed the other way round for energy instead?
Sooner or later we might break the backward compatibility. I think it’s
better to do it all at once for clarity and take the occasion to fix all
these inconsistencies. Maybe we can keep track of all these small issues
and make it a target for plumed 3.0 eventually
Il giorno lun 15 mag 2023 alle 09:36 Gareth Tribello <
***@***.***> ha scritto:
@GiovanniBussi <https://github.com/GiovanniBussi>
I'm still not totally clear but I don't think my question was very clear.
I'll give it another go.
So when we pass the energy to PLUMED atoms does this:
void Atoms::setEnergy(const TypesafePtr & p) {
plumed_massert( dataCanBeSet,"setEnergy must be called after setStep in MD code interface");
MD2double(p,md_energy);
md_energy*=MDUnits.getEnergy()/units.getEnergy();
energyHasBeenSet=true;
}
Energy is thus saved in a variable that is local to PLUMED. The MD code's
energy cannot be modified as we don't even save a pointer to the variable
in the MD code that stores the energy.
When we pass an ExtraCV, PLUMED does this:
std::map<std::string,TypesafePtr> extraCV;
void Atoms::setExtraCV(const std::string &name,const TypesafePtr & p) {
mdatoms->setExtraCV(name,p);
}
void setExtraCV(const std::string &name,const TypesafePtr & p) override {
p.get<T>(); // just check type and discard pointer
extraCV[name]=p.copy();
}
It strikes me that, in theory, we could do this instead:
std::map<std::string,double> extraCV;
void Atoms::setExtraCV(const std::string &name,const TypesafePtr & p) {
double extracv; MD2double(p,extracv);
extraCV[name] = extracv;
}
This would be more like what was done for energy. Furthermore, as with
energy, if done this way you can pass the value of the extraCV directly
instead of the pointer.
A reason for not doing it this way is if you want PLUMED to be able to
change the value of the extracv in the MD code but you don't want to do
that so... why the difference from the way energy is set?
—
Reply to this email directly, view it on GitHub
<#933 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABBNCXT4TYGFIZNTLZTGUNLXGHMGXANCNFSM6AAAAAAX2F6TWY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Sent from Gmail mobile
|
|
Another reason is that I was initially planning to implement vector extra
CV. But I never did it in the end.
Il giorno lun 15 mag 2023 alle 10:13 Giovanni Bussi <
***@***.***> ha scritto:
I see the point. I think I did it for consistency with positions (even
though this is scalar so it was not necessary), and you correctly argue the
inconsistency with energy.
One important fact is that now this is crystallized in the gromacs patch,
see attachment: basically you can compute the bias multiple times while
doing the Monte Carlo on lambda, and setExtraCV is not called every time.
If we change it to pass by value it will break this.
Is there any specific reason (except for consistency with the energy) to
do so? Or could it be changed the other way round for energy instead?
Sooner or later we might break the backward compatibility. I think it’s
better to do it all at once for clarity and take the occasion to fix all
these inconsistencies. Maybe we can keep track of all these small issues
and make it a target for plumed 3.0 eventually
Il giorno lun 15 mag 2023 alle 09:36 Gareth Tribello <
***@***.***> ha scritto:
> @GiovanniBussi <https://github.com/GiovanniBussi>
>
> I'm still not totally clear but I don't think my question was very clear.
> I'll give it another go.
>
> So when we pass the energy to PLUMED atoms does this:
>
> void Atoms::setEnergy(const TypesafePtr & p) {
> plumed_massert( dataCanBeSet,"setEnergy must be called after setStep in MD code interface");
> MD2double(p,md_energy);
> md_energy*=MDUnits.getEnergy()/units.getEnergy();
> energyHasBeenSet=true;
> }
>
> Energy is thus saved in a variable that is local to PLUMED. The MD code's
> energy cannot be modified as we don't even save a pointer to the variable
> in the MD code that stores the energy.
>
> When we pass an ExtraCV, PLUMED does this:
>
> std::map<std::string,TypesafePtr> extraCV;
> void Atoms::setExtraCV(const std::string &name,const TypesafePtr & p) {
> mdatoms->setExtraCV(name,p);
> }
> void setExtraCV(const std::string &name,const TypesafePtr & p) override {
> p.get<T>(); // just check type and discard pointer
> extraCV[name]=p.copy();
> }
>
> It strikes me that, in theory, we could do this instead:
>
> std::map<std::string,double> extraCV;
> void Atoms::setExtraCV(const std::string &name,const TypesafePtr & p) {
> double extracv; MD2double(p,extracv);
> extraCV[name] = extracv;
> }
>
> This would be more like what was done for energy. Furthermore, as with
> energy, if done this way you can pass the value of the extraCV directly
> instead of the pointer.
>
> A reason for not doing it this way is if you want PLUMED to be able to
> change the value of the extracv in the MD code but you don't want to do
> that so... why the difference from the way energy is set?
>
> —
> Reply to this email directly, view it on GitHub
> <#933 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABBNCXT4TYGFIZNTLZTGUNLXGHMGXANCNFSM6AAAAAAX2F6TWY>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
Sent from Gmail mobile
--
Sent from Gmail mobile
|
|
That is all fine. I have implemented ExtraCV so that it is consistent with the way it used to be done. I just was confused as to why the mechanism for passing ExtraCV was different from the energy as it is just a scalar. I found a way to reproduce both behaviours though that I think also makes it clear that the two things are done differently. |
To get Boltzmann's constant, the units or a bool telling you if PLUMED is using natural units inside an action you used to have to include a call something like the following plumed.getAtoms().getUnits() As this is used in a lot of places I put a method in Action so you can now get the units using: getUnits() instead. I have made similar changes from: plumed.getAtoms().getKBoltzman() to getKBoltzmann() and plumed.getAtos().usingNaturalUnits() to usingNaturalUnits()
…tion and Colvar to ActionWithValue The way that you get the forces from the Values is the same in Function and in Colvar. I thus created a method called getForcesFromValues in ActionWithValue and used this method in Function and Colvar. I also rewrote the interface for the methods setForcesOnAtoms and addForcesToArguments. You now passed a reference to an unsigned which keeps track of where you are in the forcesForApply array that is passed to this functions.
ActionWithVirtualAtom now creates 5 values: the x, y and z position of the atom and the mass and charge of the atom. This replaces the mechanism whereby additional atoms were created in the Atoms class. ActionAtomistic creates vectors containing pointers to the PLMD::Values that are created by the PUT actions that pass positions from the MD code and the Virtual Atom actions. If virtual atoms are needed a dependency on the virtual atoms action is created in requestAtoms.
|
Happy new year all! I had another look at this over the last couple of days. I wrote a CLTool that generates a trajectory and then calls plumed so that you can run plumed without a load of IO. I then made a few small changes to optimise some things with apply. The timings that I get currently are as indicated in this graph.
All these tests are run in serial. Tests 1-5 are Giovanni's one. 6-9 are ones that involve apply that were created using Carlo's modifications to Giovanni's tests. I am pretty sure the code is doing better on the apply side than when Carlo was trying it, which is progress. I am not sure if the changes that speed up CENTER were moved to master yet so not totally confident about that graph. @GiovanniBussi can you clear up what the status with transferring those optimisations into master? The next step I am going to take is to look into WHOLEMOLECULES and to ensure that is not slowing things down as well. Beyond that I am not sure what to do. I am not sure how to squeeze any more performance out of the functions for apply in the htt branch and based on the graph above that is what is slowing down the new branch. |
…s WholeMolecules and WrapAround
|
Hello all I had a bit of a brainwave. Here are the new timings for the code.
Tests 10 and 11 are WholeMolecules and WrapAround. |
|
Hello @GiovanniBussi and @Iximiel I have fixed the regtest/basic/rt16 in the master-center-optimised branch as we discussed in the meeting. I guess you can now merge master-center-optimised into master and do the comparison between master and this branch to discuss the next steps for this branch. At some stage in the next few days I will try to resolve the issues with the checks that are appearing here. I haven't worried about that too much while we have been trying to make the code faster but now that we are close to merging it would be good to not have these tests failing. Here are the set of plumed input files that I was using for benchmarking. |
|
I did some performance tests with this, that is my take on a Giovanni's gist. I wanted to use perf to generate the flamegraphs, so I used I only confronted using a similar configuration of your "#test 1", with centers calculated on 1000 and 100000 atoms CONTIGUOS in memory (it may be interesting to see how a I confronted three runs:
public:
bool chargesWereSet;
private:
If I confront the commit 2b80592 to the december one: there is more or less an halving in time of the backward loop 👍, that now is only 10% slower than the one in master. But if you observe better, is actually faster for some of the heavier actions, look at c1 and c0. However center runs slower on both the htt versions, and by looking at the flamegraphs my interpretation is that now the compiler (at least gcc 9.4.0 on my workstation) finds more difficult to understand what we actually want to do with the code: because on the master some avx instructions pop out during center::calculate. I did not studied this further, because the major slowdown in December was in the backward loop, I'll look at this in these days. DataMaster (8d8e526)
HTT (2b80592)
HTT (December)
How to read a flamegraphThe more height the more depth is in that stack call, the more is the rectangle larger, the more time has passed in the stack, function are in alphabetical order, largeness of rectangle=%time passed in the stack and the colors distiguish the functions, the palette is set up to use the same color for the same named function in different graphs. |
| ato=matmul(rotation,ato); | ||
| unsigned nat = getTotAtoms(); | ||
| for(unsigned i=0; i<nat; i++) { | ||
| std::pair<std::size_t,std::size_t> a = getValueIndices( AtomNumber::index(i)); |
Check notice
Code scanning / CodeQL
Declaration hides variable Note
|
@GiovanniBussi do you want to push the button on this or are you happy for me to do it? |
|
Please go ahead! 😀 |
|
@gtribello @GiovanniBussi nice this is merged! I have noticed that now gromacs tells but this is not true because METAD knows the value of KbT, so there is a small bug somewhere Furthermore, |
|
@carlocamilloni can you provide a screenshot showing that parsing keywords takes significant time? I tried to see that with Instruments but without success... |
|
@carlocamilloni thanks for checking this. I had a look at the issue with the dumping of KbT. There was a typo in the place where it was output to the log. It should be fixed now. |
Sure, I will try to repeat the analysis, I also noticed that instruments does not always manage to show all functions in a relative unpredictable way |
|
@GiovanniBussi @gtribello this is the report from instruments on the aladp run: |
|
you can see that getWords uses the 0.9% of the time, that I would say is quite significant and it was 0 in the past you can download the trace here: On linux I have some troubles in opening the trace with this format, can you export a .csv version? |
|
Thanks @carlocamilloni I see, I can reproduce this with a dummy test, just using a very low number of atoms (e.g. 20) as you have in alanine dipeptide in vacuum. I think it can (should) be optimized, I will take care of it |
@carlocamilloni I tried to visualize it with the flamegraph, but its instruments interface works only with a csv... |
Description
As discussed a couple of weeks ago I am starting the process of transferring functionality from the hack-the-tree branch into the master branch of PLUMED. This commit includes code that generalises the mechanisms for passing from/to the calling code to/from PLUMED. I have written some notes explaining the rationale for these changes here:
https://plumed-school.github.io/lessons/23/001/data/MDInterfaceI.html
As I continue this process of trying to merge hack-the-tree into master I will write further posts here:
https://plumed-school.github.io/lessons/23/001/data/NAVIGATION.html
Currently, the new mechanism is used to pass:
I plan to see what you folks see of this stuff and then work on trying to also pass the positions in the way this is done in hack-the-tree.
Target release
It would be nice if this could appear in v2.10
Type of contribution
Copyright
COPYRIGHTfile with the correct license information. Code should be released under an open source license. I also used the commandcd src && ./header.sh mymodulenamein order to make sure the headers of the module are correct.Tests