Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compacting the data structure within PLMD::Tree #1073

Open
Iximiel opened this issue May 9, 2024 · 1 comment
Open

Compacting the data structure within PLMD::Tree #1073

Iximiel opened this issue May 9, 2024 · 1 comment

Comments

@Iximiel
Copy link
Member

Iximiel commented May 9, 2024

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants