mosdef-hub / mbuild

A hierarchical, component based molecule builder
https://mbuild.mosdef.org
Other
171 stars 80 forks source link

Add 2d checkered pattern #1118

Closed daico007 closed 1 year ago

daico007 commented 1 year ago

PR Summary:

Add new 2D checkered patter to mBuild pattern.py. Grid2DPattern:

Screenshot 2023-05-27 at 16 16 56

Checkered2DPattern:

Screenshot 2023-05-27 at 16 17 18

PR Checklist


codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 96.00% and project coverage change: +0.03 :tada:

Comparison is base (5e505ab) 87.13% compared to head (7767544) 87.16%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1118 +/- ## ========================================== + Coverage 87.13% 87.16% +0.03% ========================================== Files 62 62 Lines 6420 6445 +25 ========================================== + Hits 5594 5618 +24 - Misses 826 827 +1 ``` | [Impacted Files](https://app.codecov.io/gh/mosdef-hub/mbuild/pull/1118?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub) | Coverage Δ | | |---|---|---| | [mbuild/pattern.py](https://app.codecov.io/gh/mosdef-hub/mbuild/pull/1118?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mosdef-hub#diff-bWJ1aWxkL3BhdHRlcm4ucHk=) | `95.56% <96.00%> (+0.08%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

chrisiacovella commented 1 year ago

A few comments:

let us ditch the "row" and "column" terminology in the Checkered2DPattern and Grid2DPattern. It would be more consistent (with the 3d packing) and clearer to say:

    n : int
         Number of points along x-axis

    m : int
         Number of points along y-axis

Similarly, I think change the accepted values for shift to be either 'n' or 'm' (or 'x' or 'y') where shift='n' would mean shifting occurs in the x-direction (shifting every other row in 'm').

To make it clear what is being done, I think the description in the docstring should probably state what is done:

Generates a square grid of dimensions n by m, then shifts points accordingly to generate a triangular arrangement. shift='n' would mean shifting occurs in the x-direction, where the code shifts every other row in the range specified by 'm'. Similarly shift='m' performs shifting along the y-direction, where the code shifts every other column in range specified by 'n'. By default, shifting will be half the distance between neighboring points in the direction of shifting. This code will allow patterns to be generated that are only periodic in one direction; to generate a periodic pattern, m should be even when shift='n' and 'n' should be even when shift='m'.

To generalize, could also allow the value of the delta_shift to be set; this allows of course a wider range of grids to be defined, but also if delta_shift is set to zero, it would recover grid2dpattern. As such, we should probably just have Grid2DPattern inherit from this class, setting delta_shift=0.

I think Triangle2DPattern might be a more appropriate name than checkered, since a square lattice can be checkered by alternately labeling the points (which might actually be a useful addition to the code in a future PR, being able to label the point and thus being able to pass a point label name to the apply function so that only select points would get compounds moved to them) .