More features from hack-the-tree #1033
Conversation
…how this is matrix vector multiplication
…required are not compiled
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if( getNumberOfComponents()==2 && getPntrToComponent(1)->forcesWereAdded() ) { | ||
| unsigned natoms = getPntrToArgument(0)->getShape()[0] / 3; | ||
| std::vector<Vector> pos( natoms ), der( natoms ), direction( natoms ); | ||
| double r = calculateRMSD( 0, pos, der, direction ); |
Check notice
Code scanning / CodeQL
Unused local variable Note
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1033 +/- ##
==========================================
- Coverage 84.08% 83.37% -0.72%
==========================================
Files 623 618 -5
Lines 58949 59099 +150
==========================================
- Hits 49569 49272 -297
- Misses 9380 9827 +447 ☔ View full report in Codecov by Sentry. |
| @@ -1,3 +1,4 @@ | |||
| plumed_modules=adjmat | |||
There was a problem hiding this comment.
@gtribello why does this need adjmat?
There was a problem hiding this comment.
I will try to explain this here but it would be good to have a chat about how to proceed here. My preference would be to move some files around a little bit and then submit another PR.
So a plumed input file that could be created for doing DRMSD (remember DRMSD is a shortcut) is as follows (it is not done this way. Although the way described below is probably better. That is another change that should perhaps be made).
# Create a vector to hold the reference distances
dref: CONSTANT VALUES=1.1,1,3,1.4....
# Calculate all the distances
d: DISTANCES ATOMS1=1,2 ATOMS2=3,4 ...
# Calculate the displacement vector
dis: DISPLACEMENT ARG=d,dref
# Transpose the displacement vector
disT: TRANSPOSE ARG=dis
# Matrix multiply the transpose by the displacement to get the distance
d: MATRIX_PRODUCT_DIAGONAL ARG=disT,dis
This uses functionality for dealing with matrices. All of this functionality is currently in adjmat as I wrote all the matrix stuff initially with adjacency matrices in mind. I then started using it in other places because matrix multiplication is really useful! So I should probably have a rethink of how the files are organised into modules. That is quite a big job though that should maybe be done in a different PR.
There was a problem hiding this comment.
One issue that I see with the shortcuts is the following.
In a recent commit e912958 I used a short script to remove unneeded dependencies. The script looks like this:
for file in */Makefile ; do
if grep -q "USE=" $file ; then
deps=$(grep USE= $file | awk '{for(i=2;i<=NF;i++) print($i "/")}')
for dep in $deps ; do
git grep -q "^ *# *include \"$dep" ${file%Makefile} || echo ERROR $file $dep
done
fi
done
It checks if a module was USEd by mistake. And the fact that the build with all modules enables (on GitHub Actions) confirms that all needed modules where USEd.
However, we now have other dependencies that are a bit more difficult to track. For instance, the code might build with a set of modules, some action would seem available, but then would lead to an error at runtime because they generate a shortcut that need another module.
I don't have a solution however...
src/setup/DRMSD.cpp
Outdated
There was a problem hiding this comment.
Why is this in the setup subdirectory? Here we only keep Actions that inherits from ActionSetup. These actions are supposed to be before other actions because they affect how other action works. Such as UNITS, LOAD, or RESTART. I now see that there are many other actions in this directory now, I guess they ended up here by mistake, or is there something that I missed?
There was a problem hiding this comment.
I moved DRMSD to the setup directory because it is a shortcut. It works by creating a longer (more-complicated) input file that uses other action. Consequently, functions in the DRMSD class are only called during setup. I can move this back to Colvar if you prefer.
The other things that I put in setup are Constant.cpp, Ones.cpp and PDB2Constant.cpp. These all create constants (Ones.cpp and PDB2Constant.cpp are actually shortcuts that create Constant actions). In other words, these actions create a value during setup that then does not change during the simulation. I think it makes sense for these to be in setup.
There was a problem hiding this comment.
I see. I noticed now that with commit 79c832a (the big merge) you actually changed the meaning of ActionSetup.
Before: an action that should be at the beginning of the file because it changes how other actions are processed. Think about UNITS
Now: this is true only if you don't have a LABEL keyword
The point of having a separate base class was to enforce this with dynamic cast.
Maybe could you restore this? I guess you can simply use an empty calculate function if the purpose is to avoid calculations, right?
|
@gtribello I added a few comments inline. I made a quick test and I noticed that RMSD is faster than before. I think that the reason is that you avoid using the reference module now, and directly use the tool (as it was in the earliest versions). BTW I noticed that RMSD gives a segmentation fault using the benchmark trajectories after the 4th step. There might be a bug somewhere, but it looks like also previous versions of plumed have the same problem, so it's likely unrelated to this PR. |
|
NB: the segmentation fault I was mentioning was a real bug, only showing up when all the atoms are arranged in a straight line (which happens in the benchmark trajectories). This is fixed in this commit now: f1da0a9 |
Description
These commits contain the new implementations of the actions belonging to the mapping module. This is PATH, GPROPERTYMAP, PCAVARS, pathtools, ADAPTIVE_PATH and the GEOMETRIC_PATH. These functionalities can be simplified because we can now pass vectors and matrices between actions.
I think there is less controversial stuff here as I am mostly changing code that I authored. The only action I have written that was not authored by me is RMSD. This now calls the Tools::RMSD class directly (rather than using the reference module). Furthermore, I have made the command RMSD a shortcut so colvar::RMSD is now called RMSD_SCALAR. To be clear, however, you can still call RMSD with the old syntax:
The difference is that if
myref.pdbcontains a list of reference structures you call a class called RMSD_VECTOR that was authored by me that calculates a vector of RMSD values from the reference structures. Alternatively, if you do:Then RMSD_VECTOR is called again as you are calculating the vector of RMSD displacements for each atom.
Target release
I would like my code to appear in release 2.10 (ideally but I understand if that is too big an ask).
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