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

Update simulation interface and add density to update_volume #42

Merged
merged 42 commits into from
Sep 15, 2023
Merged

Update simulation interface and add density to update_volume #42

merged 42 commits into from
Sep 15, 2023

Conversation

marjanalbooyeh
Copy link
Collaborator

This PR applies a couple of changes to the system and simulation classes:

  • A simulation object can be instantiated in two ways now: 1) From a system 2) From an initial state and a list of forces
  • run_update_volume in simulation now takes in a density parameter which basically calculates the final box length given the density and shrinks or expands based on that.
  • target_box in system will be scaled by the reference length.
  • A new method called calculate_box_length has been added to utils, which calculates the target box length given the mass and density. Both system and simulation classes call this method now.

P.S. In calculate_box_length method, I'm not sure if the mass conversion from amu to g is needed for all cases or not? This method is called in a few scenarios:
1- During system initialization where no scaling has happened yet
2- When run_update_volume is called from the simulation. This can be on a scaled or unscaled system.

Once we agree on these changes, I will update the unit tests and docstrings.

Copy link
Member

@chrisjonesBSU chrisjonesBSU left a comment

Choose a reason for hiding this comment

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

I left a couple small change requests.

hoomd_organics/base/simulation.py Outdated Show resolved Hide resolved
hoomd_organics/base/simulation.py Outdated Show resolved Hide resolved
hoomd_organics/base/simulation.py Show resolved Hide resolved
hoomd_organics/utils/utils.py Outdated Show resolved Hide resolved
hoomd_organics/utils/utils.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #42 (332b46a) into main (4a626f9) will increase coverage by 1.40%.
The diff coverage is 97.10%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #42      +/-   ##
==========================================
+ Coverage   92.42%   93.83%   +1.40%     
==========================================
  Files          18       18              
  Lines        1321     1347      +26     
==========================================
+ Hits         1221     1264      +43     
+ Misses        100       83      -17     
Files Changed Coverage Δ
hoomd_organics/base/system.py 89.51% <95.45%> (-0.28%) ⬇️
hoomd_organics/base/simulation.py 91.96% <97.29%> (+6.43%) ⬆️
hoomd_organics/utils/__init__.py 100.00% <100.00%> (ø)
hoomd_organics/utils/utils.py 100.00% <100.00%> (ø)

@chrisjonesBSU
Copy link
Member

Since this PR makes quite a bit of changes to simulation.py maybe it would be a good time to use the validate_ref_value function in the ref value setters, just like we have in system.py?

@marjanalbooyeh marjanalbooyeh merged commit 15fd092 into cmelab:main Sep 15, 2023
4 checks passed
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.

2 participants