Added Functionalized pores with percent fills. #14
Conversation
|
Looks good so far. The changes autopep8 made in |
| up_port = mb.Port( | ||
| anchor=self[0], orientation=[ | ||
| 0, 1, 0], separation=.075) | ||
| self.add(up_port, "up") |
There was a problem hiding this comment.
I think this should be named down. It's not a strict requirement but it's a convention from mb.Polymer that I would like to continue for simple systems.
| self.children[1], | ||
| self.children[1].labels['up'], | ||
| self.children[0].labels['up']) | ||
| up_port = mb.Port( |
There was a problem hiding this comment.
I think this port should be in O
porebuilder/porebuilder.py
Outdated
| @@ -1,5 +1,8 @@ | |||
| import mbuild as mb | |||
| import numpy as np | |||
| from random import shuffle | |||
| from copy import deepcopy | |||
There was a problem hiding this comment.
I don't see deepcopy being used, maybe this was debug/experimental?
| @@ -1,5 +1,8 @@ | |||
| import mbuild as mb | |||
| import numpy as np | |||
| from random import shuffle | |||
| from copy import deepcopy | |||
| from six import string_types | |||
There was a problem hiding this comment.
six is not in the standard Python library, so we should add it to requirements.txt
porebuilder/porebuilder.py
Outdated
| func_ports = [func_ports] * len(func_groups) | ||
|
|
||
| if len(func_groups) != len(func_percent) or len( | ||
| func_groups) != len(func_ports): |
There was a problem hiding this comment.
How about if not len x == y == z:?
porebuilder/porebuilder.py
Outdated
| b_surface = [] | ||
|
|
||
| for C in Top: | ||
| if C.pos[0] >= 0 and C.pos[1] <= .336 * (n_sheets - 1) + pore_width: |
There was a problem hiding this comment.
I don't understand why we need to check for a positive x coordinate?
porebuilder/porebuilder.py
Outdated
| if C.pos[0] >= 0 and C.pos[1] >= .335 * (n_sheets - 1): | ||
| b_surface.append(C), | ||
|
|
||
| for side, surface in zip((Top,Bot),(t_surface,b_surface)): |
There was a problem hiding this comment.
Can we raname side to something like pore_wall? It clashes a little bit with the side_dim argument we are using elsewhere.
| 0, 1, 0], separation=.075) | ||
| self.add(up_port, "up") | ||
|
|
||
| with pytest.raises(ValueError): |
| func_groups=[H(), O()], func_percent=[.03, .04, .03]) | ||
|
|
||
| for per in range(1, 10): | ||
| pore = pb.GraphenePoreFunctionalized( |
There was a problem hiding this comment.
-
This is a fairly long test, maybe switch it to only 3 values of
per? -
We should test this for different values of
pore_widthto verify the process of finding the surfaces. Two or three values should be enough
| for per in range(1, 10): | ||
| pore = pb.GraphenePoreFunctionalized( | ||
| func_groups=H(), func_percent=per / 10) | ||
| assert(pore.n_particles - 2688 > (per - .3) / 10 * |
There was a problem hiding this comment.
Let's split this into two assert statements. And I'm not sure where 864 comes from?
There was a problem hiding this comment.
864 is the total number of inner surface carbons that could potentially be functionalized. Now that I changed the function so that it adds exactly the right amount of new groups instead of relying on dice rolls I'll be re-writing the test cases next.
|
It doesn't need to be included in this PR, but it occurred to me that code finding the surface atoms would be useful for other things (i.e. applying a partial charge) and could be moved into a separate function. |
|
@gawquon can you rebase? The master branch is passing on Travis now Something like this should work |
|
I forgot where we were with this but rebasing seems to have fixed the tests. Does this PR look okay to you @mattwthompson? |
|
I think my comments from last summer still need fixing but overall it's good. |
|
@gawquon @mattwthompson Commenting here so this can get picked back up and merged soon. |
More tests to be added later. Hopefully should be PEP8 compliant; I ran autopep8 with --in-place -a -a.