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.

Sign up for GitHub

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

Jump to bottom

Update simulate.py #179

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Update simulate.py #179

wants to merge 9 commits into from

Conversation

moyuanchen
Copy link

updated Brownian simulation to keep track of radius instead of mass of particles.

@moyuanchen
Copy link
Author

this commit fix #177

@moyuanchen
Update simulate.py
8b04205 
Added options to pass in key word arguments to the update_sigma_fn in Brownian Dynamics simulation
@cpgoodri
Copy link
Contributor

I have a few unorganized thoughts

  • As I understand it, @moyuanchen is correct in BD Documentation Clarification, and suggestions  #177 that the definition of gamma in the original implementation had a 1/mass in it, and this was not documented.
  • I'm don't have strong opinions about which definition of gamma to use. I'd lean towards the one without mass but making this switch would break backwards compatibility.
  • The ability to specify the dynamic viscosity and sigma is a nice feature because then people don't have to implement stokes law themselves (though it is a 1-liner).
  • However, the change implemented bakes in a lot of assumptions (e.g. that the particles are spherical) that I don't think we want.
  • A possible solution would be to provide a helper function that takes a dynamic viscosity and a sigma and returns gamma. Users could then use this when calling the brownian routine.
  • This PR also introduces the idea of having time-varying sigma, which more to the point means a time varying gamma. I think this is a good idea, but I think there might be better ways of implementing it, e.g. though something along the lines of how time-varying temperature is implemented. @sschoenholz, is there a reason to apply static_cast to gamma other than a marginal speedup that would prevent reading in gamma from keyword args?

@moyuanchen moyuanchen closed this Feb 21, 2022
@moyuanchen moyuanchen reopened this Feb 21, 2022
@moyuanchen
Update documentation of mass and gamma
de8d4fa 
In response to @cpgoodri 's comment. Rolling back to the original definition to ensure backward compatibility, and updated the documentation to clarify the definition of gamma. Also, added the option for user to pass in gamma in the apply_fn similarly to how temperature can be passed in.
@moyuanchen
Update quantity.py
afa2d1b
@moyuanchen
Update simulate.py
5e60f3b
sschoenholz
sschoenholz approved these changes Apr 15, 2022
View reviewed changes
Copy link
Collaborator

@sschoenholz sschoenholz left a comment

Choose a reason for hiding this comment

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

This looks great. Sorry I'm getting to the PR so late, it arrived during our performance review season and I'm just catching back up now. Thanks also for your comments @cpgoodri. I had one very minor comment about the naming of the function in quantity.py, but otherwise I think this change looks great.

As a final minor point, can you try to keep column lengths to within 80 characters? Thanks!

jax_md/quantity.py
@@ -470,3 +470,15 @@ def update_fn(state: PHopState, position: Array) -> PHopState:
return PHopState(buff, phop) # pytype: disable=wrong-arg-count

return init_fn, update_fn

def gamma_from_stokes_law_3d(eta, radii, mass):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to rename this to be something a little bit more descriptive for someone who doesn't know what gamma is? Perhaps something like spherical_stokes_friction? This isn't really my area of expertise, so feel free to pick something else!

@sschoenholz
Copy link
Collaborator

Also, it seems like you'll need to sign the CLA before I am able to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sschoenholz sschoenholz sschoenholz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@moyuanchen @cpgoodri @sschoenholz