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

Add option to select MSD algorithm #291

Merged
merged 12 commits into from
Apr 3, 2024
Merged

Conversation

SCiarella
Copy link
Contributor

This PR closes #285.

Speed up trajectory._unwrap_pbc
@SCiarella SCiarella linked an issue Mar 20, 2024 that may be closed by this pull request
@SCiarella SCiarella marked this pull request as draft March 20, 2024 15:47
@SCiarella
Copy link
Contributor Author

This PR also closes #290

@SCiarella SCiarella marked this pull request as ready for review March 26, 2024 10:01
@SCiarella SCiarella requested a review from stefsmeets March 26, 2024 10:23
@SCiarella
Copy link
Contributor Author

Refer to this notebook for an overview of this PR

@stefsmeets
Copy link
Contributor

Can you say a little bit what this PR does?
What is the effect of the jit on the performance?

@SCiarella
Copy link
Contributor Author

The goal of this PR is to allow the user to select the desired algorithm to measure the MSD and the MSAD. The same choice is made available for the plotting functions in gemdat.plot.

For the MSD the user can select how many 'starting points' to use via nstarts. The default is to use all the possible starting points, in which case GEMDAT will rely on _fft_mean_squared_displacement. If the user selects a specific value of nstarts, then it is not possible to use the FFT method, so I have implemented _direct_mean_squared_displacement. However, the direct method become extremely slow for large values of nstarts, so I used jit compilation via numba, which makes the code up to 100 times faster (depending on the value of nstarts). The jit compilation requires _direct_mean_squared_displacement to be outside of Trajectory.

The same approach has been implemented for the MSAD (and plots.unit_vector_autocorrelation), where the user can select between the FFT calculation or a direct one. The difference is that instead of nstarts, the MSAD is parallelized over n_tgrid which corresponds to the specific values of $t$ where the MSAD should be measured (the MSAD always assumes nstarts=all).

Additionally, this PR also addresses some minor improvements, like implementing the time units on the msd plots, normalizing the autocorrelation and plotting the error.

Copy link
Contributor

@stefsmeets stefsmeets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SCiarella just made a first pass. Plots look great as always 🚀

I'm not entirely convinced that we need numba over some vectorized numpy code (see comments).

I also do not understand why we need nstarts as an argument it adds, for me it complicates things. What would be the most optimal way to calculate the msd? That should be the one we support.

Finally, can you add some unit tests (not integration) for the new code?

@@ -298,45 +299,93 @@ def calculate_spherical_areas(shape: tuple[int, int],
return areas


def autocorrelation(trajectory: np.ndarray) -> tuple[np.ndarray, np.ndarray]:
"""Compute the autocorrelation of the trajectory using FFT.
def mean_squared_angular_displacement(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me like this should be a method on Trajectory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the input trajectory for mean_squared_angular_displacement is a unit vector trajectory that gets returned from Orientations directly as a np.ndarray. We should address the output of Orientations in a future PR.

src/gemdat/rotations.py Outdated Show resolved Hide resolved
src/gemdat/rotations.py Outdated Show resolved Hide resolved
src/gemdat/rotations.py Outdated Show resolved Hide resolved
src/gemdat/rotations.py Outdated Show resolved Hide resolved
src/gemdat/trajectory.py Outdated Show resolved Hide resolved
src/gemdat/trajectory.py Outdated Show resolved Hide resolved
@SCiarella
Copy link
Contributor Author

Update:

After the optimization happening in this PR, GEMDAT will support only the most efficient methods for the MSD and the MSAD, which are the FFT methods.

The results of a comparison available here show that:
(1) the MSD direct method converges to the FFT results
image
(2) the FFT method for MSD is faster
image

(3) the MSAD direct method converges toward the FFT results
image
(4) the FFT method for MSAD is faster
image

@SCiarella
Copy link
Contributor Author

This PR closes #274

@SCiarella SCiarella marked this pull request as draft March 29, 2024 09:35
@SCiarella SCiarella marked this pull request as ready for review March 29, 2024 10:28
This was linked to issues Mar 29, 2024
@SCiarella SCiarella requested a review from stefsmeets March 29, 2024 10:40
Copy link
Contributor

@stefsmeets stefsmeets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@stefsmeets stefsmeets merged commit a5ff972 into main Apr 3, 2024
5 checks passed
@stefsmeets stefsmeets deleted the 285-accelerate-msd-calc branch April 3, 2024 13:39
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

Successfully merging this pull request may close these issues.

autocorrelation should use unit vectors accelerate MSD calc. MSD enhancements
2 participants