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

add biased voronoi cells feature #593

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

add biased voronoi cells feature #593

wants to merge 35 commits into from

Conversation

yuanzhou0827
Copy link

@yuanzhou0827 yuanzhou0827 commented Mar 6, 2020

Description

Add a new possibility to have volume-polydisperse/weighted voronoi diagram.

Motivation and Context

Have weighted voronoi diagram available, thus we can do binary assembly from size-weighted voronoi cells.

Resolves: #???

How Has This Been Tested?

Screenshots

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds or improves functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • I have updated the documentation (if relevant).
  • I have added tests that cover my changes (if relevant).
  • All new and existing tests passed.
  • I have updated the credits.
  • I have updated the Changelog.

@yuanzhou0827 yuanzhou0827 requested a review from a team as a code owner March 6, 2020 22:57
@glotzerlab glotzerlab deleted a comment from codecov bot Mar 7, 2020
@codecov
Copy link

codecov bot commented Mar 7, 2020

Codecov Report

Merging #593 (d4e5e6c) into master (e3eeb30) will decrease coverage by 0.02%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #593      +/-   ##
==========================================
- Coverage   55.24%   55.21%   -0.03%     
==========================================
  Files          15       15              
  Lines        2576     2581       +5     
  Branches       38       38              
==========================================
+ Hits         1423     1425       +2     
- Misses       1137     1140       +3     
  Partials       16       16              
Impacted Files Coverage Δ
freud/locality.pyx 39.41% <42.85%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3eeb30...d4e5e6c. Read the comment docs.

@bdice
Copy link
Member

bdice commented Mar 13, 2020

@yuanzhou0827 Let me know if you have questions about this PR!

Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

@yuanzhou0827 I commented on the type issues you had -- you were mixing single precision (float, np.float32_t and double precision (double, np.float64_t) values. Also it seems like master wasn't merged into this branch correctly. Let me know if you want help with that.

cpp/locality/LinkCell.cc Outdated Show resolved Hide resolved
freud/locality.pyx Outdated Show resolved Hide resolved
freud/locality.pyx Outdated Show resolved Hide resolved
freud/locality.pxd Outdated Show resolved Hide resolved
freud/locality.pyx Outdated Show resolved Hide resolved
@bdice bdice added this to the v2.3 milestone Mar 19, 2020
@bdice bdice added enhancement New feature or request voronoi labels Mar 19, 2020
@yuanzhou0827 yuanzhou0827 requested a review from bdice March 19, 2020 18:02
Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Good progress, @yuanzhou0827! We need to add tests for this code, and expand on the documentation. Some of the tests you can add:

  • Create a random system of 1000 points and box side length 10, with random radii between 0 and 1. Make sure the sum of the volumes is approximately equal to the box volume.
  • Manually construct a small system (even with just 2 points) where the weights are unequal. Verify as many properties (volumes, polyhedra coordinates, weights, bonds) as you can derive.

cpp/locality/Voronoi.cc Show resolved Hide resolved
cpp/locality/Voronoi.cc Outdated Show resolved Hide resolved
@@ -1120,17 +1120,28 @@ cdef class Voronoi(_Compute):
def __dealloc__(self):
del self.thisptr

def compute(self, system):
def compute(self, system, radii=None):
R"""Compute Voronoi diagram.

Copy link
Member

Choose a reason for hiding this comment

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

We need to add a longer description of Voronoi diagrams and how this feature works. I think it might go best in the class docstring. It should include external links, formulas/equations, and descriptions of why someone would want to use this feature. It's important to include the several names for this feature so that it can be found via Google / documentation search. Please verify what type of weighted Voronoi diagram we are computing (additively weighted, multiplicatively weighted, or something else?), and cite whatever references you can find. You may need to refer to the voro++ documentation.

freud/locality.pyx Outdated Show resolved Hide resolved
freud/locality.pyx Outdated Show resolved Hide resolved
@bdice bdice modified the milestones: v2.3, v2.4 Aug 1, 2020
@bdice bdice modified the milestones: v2.4, v2.5 Nov 16, 2020
@bdice bdice removed this from the v2.5 milestone Mar 26, 2021
@bdice
Copy link
Member

bdice commented Nov 21, 2021

@yuanzhou0827 Have you used this feature in your research? If so, we should try to finish and merge it. If you haven't used this feature in your research then I suggest we close this pull request.

@yuanzhou0827
Copy link
Author

@bdice Hey Bradley, surely i want to finish it asap. I tried some day before but have been struggling with the version conflicts. I am thinking if it is possible to close this PR and make the changes in the newest version freud? That would be easier and won't cost too much time because there are only several-lines-changes?

@bdice
Copy link
Member

bdice commented Nov 21, 2021

@yuanzhou0827 I just did a merge commit (c1d032a) so this PR is up to date with the latest changes in freud. If you'd like to finish it, just pull those changes and you'll be up to date!

I think the items needed on this PR are documentation (see this comment for some ideas) and tests (see this file for the existing Voronoi tests, just add your own tests for particles with radii in a similar style).

@yuanzhou0827
Copy link
Author

cool! Thank you! i will work on the test cases and documentations in days!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request voronoi
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants