plumed / plumed2

Development version of plumed 2
https://www.plumed.org
GNU Lesser General Public License v3.0
323 stars 269 forks source link

Compacting the data structure within PLMD::Tree #1073

Open Iximiel opened 1 month ago

Iximiel commented 1 month ago

I wanted to add the following comment to #1068, but after writing it felt out of scope for the PR, since these changes are more a refactor than a change in behavior that is the target of #1068:

I think it may be a good idea to modify the PLMD::Tree class with

template<typename T>
struct leaf{
//T will be unsigned and AtomNumber
T index;
T root;
}

so you can use only a std::vector<leaf<unsigned>> tree_indexes_ and one std::vector<leaf<AtomNumber>> tree_

and then you can iterate (for example in ActionAtomistic::makeWhole())

    for(const auto&[tree, root] : tree.getTreeIndexes()) {
      const Vector & first (positions[root]);
      Vector & second (positions[tree]);
      second=first+pbcDistance(first,second);
    }

that has the advantage of producing less instructions and working with adjacent data in memory, and should be slightly faster.

What do you think?

If you are ok with this, I would prepare a PR and measure if I can see any speedup after #1068 is merged

GiovanniBussi commented 1 month ago

Yes it makes sense, it is basically merging the two vectors in a single vector of pairs.

Let's keep this issue open, but wait a second because I am also trying to see if I can fill the last missing bit, which is doing EMST on the fly. It would be super expensive but also super useful in analysis, so maybe it's worth.