cmelab / polybinder

Initialization of thermoplastic polymer systems to simulate thermal welding
GNU General Public License v3.0
2 stars 5 forks source link

Revamp system and simulation initialization #47

Closed chrisjonesBSU closed 3 years ago

chrisjonesBSU commented 3 years ago

This PR does a few things. The main goals are to have more options in system generation and simulation ensemble choices.

Summary of changes made:

  1. Break simulate.py into systems.py and simulate.py
  2. Add Initialization() class to systems.py
  3. Add NPT emsemble functionality when starting a simulation

The Initialization() class: systems.py has 2 main classes (a 3rd one for initializing slab/interfaces, but that's kind of a separate thing).

  1. System() This remains kind of the original system initialization class that was used before. It handles and tracks all of the system state point information that is required for initializing a system.

  2. Initialize() This new class is a place to hold different system initialization schemes (functions). For example, the mbuild fill box logic was moved within this class under a pack function, but there is also a different function, stack that layers the compounds one after another rather than using the packing.py functionality in mbuild. This will be helpful for low density simulations where the goal is to model 1 or 2 molecules interacting.

Overall, I think this approach makes the system initialization step much more extensible. If/when new schemes are needed, the Initialization class can be extended with a new function to handle them without interfering with the existing initialization schemes.

NPT Ensembles: The Simulation class now has a tau_p parameter, and both the quench and anneal have pressure parameters.
Switching from NVT to NPT after the shrink step is handled automatically.

Still a couple things to do:

codecov[bot] commented 3 years ago

Codecov Report

Merging #47 (ccda71a) into master (27c3384) will decrease coverage by 0.76%. The diff coverage is 93.30%.

:exclamation: Current head ccda71a differs from pull request most recent head e8e61d7. Consider uploading reports for the commit e8e61d7 to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
- Coverage   94.91%   94.15%   -0.77%     
==========================================
  Files           4        5       +1     
  Lines         374      445      +71     
==========================================
+ Hits          355      419      +64     
- Misses         19       26       +7     
Impacted Files Coverage Δ
uli_init/systems.py 93.04% <93.04%> (ø)
uli_init/simulate.py 94.38% <94.87%> (+0.01%) :arrow_up:
chrisjonesBSU commented 3 years ago

A couple more things that come to mind.

1: The Systems class should probably have a kwargs parameter that can be passed to the individual functions inside of Initialize()

2: Maybe there should be functionality to accept a custom made system? For example, this might be useful for some edge cases where someone takes the time to separately create a specific system, but still want to utilize the rest of the workflow with it as normal. Rather than going through the pre-defined initialization schemes, the system is just loaded from a file. There are a couple variables that are determined when using the pre-defined initialization schemes though (mass and target box length); maybe these would just have to be passed in explicitly if you're using this approach?

chrisjonesBSU commented 3 years ago

OK I left a couple of suggestions in there--I didn't get to the changes in systems.py too deeply and I haven't had a chance to play around with it yet. But this seems like a good organization change and the logic here for initializing a polymer system seems to be getting really robust! :)

Changes made for each one of the suggestions, thanks :)