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

Function that calculates centers in a trajectory. #15

Merged
merged 21 commits into from
Dec 1, 2021

Conversation

gwenwhite
Copy link
Contributor

This function calculates the centers of a trajectory and stores them into a new GSD file (called "centers.gsd" in this code specifically).
Side note: Will need to make the code account for the image if we want to use it for MSD calculations.

Copy link
Member

@jennyfothergill jennyfothergill left a comment

Choose a reason for hiding this comment

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

Nice work, @gwenwhite! This is a much needed addition!

I agree that accounting for the image will be nice. I think you could add the image like this: unwrap the particle positions, calculate the centers as they are unwrapped, then use freud.Box.get_images to get the image of the unwrapped center. I'm happy to help you with any questions you have about adding the image.

I've suggested some minor style changes in my review. I also noted some (I think unintentional?) changes to gsd_rdf (the function above your get_centers function). I am guessing these are maybe due to a merge conflict.

gwenwhite and others added 12 commits October 6, 2021 13:28
Co-authored-by: Jenny <39961845+jennyfothergill@users.noreply.github.com>
Co-authored-by: Jenny <39961845+jennyfothergill@users.noreply.github.com>
Co-authored-by: Jenny <39961845+jennyfothergill@users.noreply.github.com>
Co-authored-by: Jenny <39961845+jennyfothergill@users.noreply.github.com>
Co-authored-by: Jenny <39961845+jennyfothergill@users.noreply.github.com>
Co-authored-by: Jenny <39961845+jennyfothergill@users.noreply.github.com>
Co-authored-by: Jenny <39961845+jennyfothergill@users.noreply.github.com>
Co-authored-by: Jenny <39961845+jennyfothergill@users.noreply.github.com>
Co-authored-by: Jenny <39961845+jennyfothergill@users.noreply.github.com>
Co-authored-by: Jenny <39961845+jennyfothergill@users.noreply.github.com>
Co-authored-by: Jenny <39961845+jennyfothergill@users.noreply.github.com>
Co-authored-by: Jenny <39961845+jennyfothergill@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Oct 6, 2021

Codecov Report

Merging #15 (6caa880) into master (5052ab4) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
+ Coverage   99.50%   99.55%   +0.04%     
==========================================
  Files           5        5              
  Lines         203      224      +21     
==========================================
+ Hits          202      223      +21     
  Misses          1        1              
Impacted Files Coverage Δ
cmeutils/structure.py 100.00% <100.00%> (ø)

Co-authored-by: Jenny <39961845+jennyfothergill@users.noreply.github.com>
@gwenwhite
Copy link
Contributor Author

In regards to commit #db09303134eb4ac96c7b4fb2fbf67c3336c296e3 - Code has been tested on larger system. It worked successfully.

Copy link
Contributor

@erjank erjank left a comment

Choose a reason for hiding this comment

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

Overall this looks good. The get_centers function looks to save out a trajectory with mapped coordinates. There are some small improvements to think about down the line in test_get_centers, since right now the only sanity check is whether get_centers returns None. It's true, but it's not particularly helpful. An example, more helpful test, might be if the number of frames in the new trajectory is the same as the number of frames in the old trajectory, or if the number of centers is equal to the number of molecules. Nice work!

@erjank erjank merged commit 258e127 into cmelab:master Dec 1, 2021
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.

3 participants