draft: add a hydrogel builder #103
Conversation
adding hydrogel feature pyMBE.py - not sure why it was left out
pm-blanco
changed the title
pm-blanco
changed the title
…drogel-feature-branch
There was a problem hiding this comment.
In general, I really like the shape this feature is taking. Currently, it is restricted to only diamond lattice topologies. However, this implementation already allows to build many interesting systems, so I think that we can keep it like this for this PR and work to generalize in further steps. I have several minor comments in specific lines and some other more conceptual ones that we can discuss during our dev meeting. Thank you for your work @1234somesh!
| @@ -20,17 +19,16 @@ | |||
| import itertools | |||
| import numpy as np | |||
|
|
|||
| #pmb = pyMBE.pymbe_library(seed=42) | |||
There was a problem hiding this comment.
please remember to clean-up these comments before merging it!
| @@ -46,6 +44,8 @@ class LatticeBuilder: | |||
| function to draw the simulation box. | |||
| """ | |||
|
|
|||
| # Remove pmb when integrated to main pyMBE.py file | |||
There was a problem hiding this comment.
please remember to clean-up these comments before merging it!
| diamond_lattice = DiamondLattice(MPC,generic_bond_length) | ||
| espresso_system = espressomd.System(box_l = [diamond_lattice.BOXL]*3) | ||
| pmb.add_bonds_to_espresso(espresso_system = espresso_system) | ||
| lattice_builder = pmb.initialize_lattice_builder(diamond_lattice) # Dont need pmb when integrated to pyMBE.py |
There was a problem hiding this comment.
please remember to clean-up these comments before merging it!
| if not isinstance(diamond_lattice, DiamondLattice): | ||
| raise TypeError("Currently only DiamondLattice objects are supported.") | ||
| self.lattice_builder = LatticeBuilder(lattice=diamond_lattice) | ||
| print(f"LatticeBuilder initialized with MPC={diamond_lattice.MPC} and BOXL={diamond_lattice.BOXL}") |
There was a problem hiding this comment.
We should add an optional bool verbose argument to control when to print to screen. For example for unit testing we do not want to overflow the output screen with unnecessary prints. The verbose statement could default to True so beginners would get the print to screen and make the software more user friendly.
| del chain_topology[0] | ||
| print(chain_topology) |
There was a problem hiding this comment.
I think these lines were added for testing but please remember to remove them or otherwise the sample script breaks!
| print(chain_topology) | ||
| pmb.define_hydrogel("my_hydrogel",node_topology, chain_topology) | ||
| node_positions = pmb.create_hydrogel("my_hydrogel", espresso_system) | ||
|
|
There was a problem hiding this comment.
There seems to be an issue with the new entries in the pyMBE dataframe related with the hydrogel when reading the pyMBE dataframe. If I modify this code with the following lines, it breaks:
#Save the pyMBE dataframe in a CSV file
pmb.write_pmb_df (filename='df.csv')
#Load the pyMBE dataframe in a CSV file
pmb.read_pmb_df(filename="df.csv")
node_positions = pmb.create_hydrogel("my_hydrogel", espresso_system)
``| name(`str`): Label of the hydrogel to be created. `name` must be defined in the `pmb.df` | ||
| espresso_system(`espressomd.system.System`): Instance of a system object from the espressomd library. | ||
|
|
||
| Returns: |
There was a problem hiding this comment.
Please document the dictionary that is returned.
| del chain_topology[0] | ||
| print(chain_topology) | ||
| pmb.define_hydrogel("my_hydrogel",node_topology, chain_topology) | ||
| node_positions = pmb.create_hydrogel("my_hydrogel", espresso_system) |
There was a problem hiding this comment.
Since the dictionary returned pmb.create_hydrogel actually has more information than only the position of the nodes. What about renaming the variable node_positions to hydrogel_info?
| @@ -0,0 +1,331 @@ | |||
| import numpy as np | |||
There was a problem hiding this comment.
You need to add this test to testsuite/CTestTestfile.cmake for the CI to run it:
## In CTestTestfile.cmake
# unit tests
pymbe_add_test(PATH hydrogel_builder.py)You can also check if your test is covering all lines of code, check our wiki
|
|
||
| """ | ||
| if not self.check_if_name_is_defined_in_df(name=name, pmb_type_to_be_defined='hydrogel'): | ||
| raise ValueError(f"Hydrogel with name '{name}' is not defined in the DataFrame.") |
There was a problem hiding this comment.
This line seems not to be covered by the CI test.
adding hydrogel feature
pyMBE.py - not sure why it was left out