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

Create dynamics.py with Freud MSD functionality #9

Merged
merged 19 commits into from
Aug 11, 2021

Conversation

chrisjonesBSU
Copy link
Member

This PR puts the MSD from GSD functionality that we have floating around in various notebooks and python files into a single location.

Also, I updated get_type_position to accept a list (or iterable) of atom types and return their positions as opposed to only accepting a single atom type. I think this will offer more flexibility.

Still needs unit tests and doc strings, but I figured I'd put it up here for others to comment on.

@chrisjonesBSU chrisjonesBSU added the WIP Work in progress label Jul 9, 2021
@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #9 (ae99f24) into master (a806ed7) will decrease coverage by 0.80%.
The diff coverage is 97.36%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master       #9      +/-   ##
===========================================
- Coverage   100.00%   99.20%   -0.80%     
===========================================
  Files            3        4       +1     
  Lines           93      125      +32     
===========================================
+ Hits            93      124      +31     
- Misses           0        1       +1     
Impacted Files Coverage Δ
cmeutils/structure.py 100.00% <ø> (ø)
cmeutils/dynamics.py 96.00% <96.00%> (ø)
cmeutils/__init__.py 100.00% <100.00%> (ø)
cmeutils/gsd_utils.py 100.00% <100.00%> (ø)

Copy link
Contributor

@gwenwhite gwenwhite left a comment

Choose a reason for hiding this comment

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

One thing I noticed when testing this code is that from cmeutils import gsd_utils was providing the error:
ImportError: cannot import name 'gsd_utils' from 'cmeutils' (unknown location)

I was able to get rid of the error running this instead :
import cmeutils.cmeutils.gsd_utils

Otherwise, looks great!

@jennyfothergill
Copy link
Member

@gwenwhite that issue might be caused when cmeutils is not installed. Do you still get the error after running

pip install .

in the root directory (the top level directory for cmeutils containing setup.py)

@chrisjonesBSU
Copy link
Member Author

Question about this:

    if stop is None:
        stop = -1

We do the same thing in gsd_rdf. Wouldn't it make more sense (and be easier for someone reading the code) to make the default parameter -1 rather than None

@jennyfothergill
Copy link
Member

We do the same thing in gsd_rdf. Wouldn't it make more sense (and be easier for someone reading the code) to make the default parameter -1 rather than None

yeah... why do we do that?? I'm with you--let's switch that.

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.

LGTM! :) I was able to get a reasonable MSD from one of my planckton runs.

Because we usually have a shrink step at the beginning of our simulations, it might be cool to have some automated detection for where the box size is stable. I think it's not necessary to hold up this PR though.

Also, I really like all the changes you've added to incorporate images into our utils!

@chrisjonesBSU chrisjonesBSU merged commit 21f91e6 into cmelab:master Aug 11, 2021
@chrisjonesBSU chrisjonesBSU deleted the dynamics branch August 11, 2021 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants