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

Correct Spheropolyhedron Class #213

Merged
merged 4 commits into from
Feb 1, 2024
Merged

Correct Spheropolyhedron Class #213

merged 4 commits into from
Feb 1, 2024

Conversation

tfteague
Copy link
Contributor

@tfteague tfteague commented Jan 31, 2024

Description

There is an error in the method via which coxeter currently computes the volumes and surface area of speropolyhedra. Part of the sums of the volume & SA are based on fractions of cylinders that extend from the edges of the base polyhedron. The included angle of this cylinder is equal to the angle between the normals of the two adjoining faces, which is equal to $\pi - \phi$, where $\phi$ is the dihedral (see e.g. equation (9) of Eric, I. M., & Michael, E. (2017). Virial Coefficients and Equations of State for Hard Polyhedron Fluids). Currently the sum is calculated only as $\phi$. While this gives correct results for the test case of a cube ($\pi/2 = \pi - \pi/2$), it does not for non-orthogonal geometries.

In addition, the mean curvature of the spherobody is implemented. The (normalized) integral mean curvature is just the sum of the mean curvature of the base shape and the radius.

Motivation and Context

  • To correct the surface area and volume functions for speropolyhedra
  • To add a mean curvature property to the speropolyhedra class

Types of Changes

  • Documentation update
  • Bug fix
  • New feature
  • Breaking change1

1The change breaks (or has the potential to break) existing functionality.

Checklist:

@janbridley janbridley self-requested a review January 31, 2024 16:26
@janbridley janbridley self-assigned this Jan 31, 2024
@tfteague tfteague marked this pull request as ready for review January 31, 2024 16:34
@tfteague tfteague requested a review from a team January 31, 2024 16:34
return h_cyl + h_sphere

@mean_curvature.setter
def mean_curvature(self, value):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the change in mean curvature require a change in both the volume of the shape and the rounding radius? And does changing both values at once result in the expected behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessarily, no. One could set the rounding radius or change the volume of the underlying polyhedron independently in order to achieve a desired mean curvature. The challenge with that approach would be that not all possible (positive, real) values could be achieved. For example, if one wanted to adjust the curvature by changing the radius alone, then one could not achieve a curvature smaller than that of the underlying polyhedron. Presumably for this reason, the setter methods for volume and surface area for this class scale the object when setting a value. I've tried to match the behavior of the already established functions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good - Polyhedron/ConvexPolyhedron don't provide setters for mean curvature but I think that it makes sense for a rounded shape like this!

@tfteague tfteague merged commit 6c04e59 into master Feb 1, 2024
8 checks passed
@tfteague tfteague deleted the update_spheropolyhedron branch February 1, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants