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 error when ellipsoid axis is zero #1785

Merged
merged 8 commits into from
May 29, 2024

Conversation

janbridley
Copy link
Contributor

@janbridley janbridley commented May 16, 2024

Description

A check was added to verify values are within the expected range. I set the minimum allowed value to ELLIPSOID_OVERLAP_PRECISION (1e-6), which may or may not be the correct choice. We could do FLT_EPSILON from as well, but that may not be consistent if HOOMD_SHORTREAL_SIZE==64. I am open to suggestions about the correct value to assign.

Errors occur then any semimajor axis is less than 0.0f.

Motivation and context

Overlap checks do not work for ellipsoids with any semimajor axis less than zero. This error (on the c++ level) prevents users from inadvertently encountering this issue.

Resolves #1762

How has this been tested?

A new test has been added that attempts to set invalid parameters. This tests values at zero, slightly above zero, and well below it.

Change log

- Provide an error message for invalid Ellipsoid shape params.

Checklist:

@janbridley janbridley marked this pull request as ready for review May 16, 2024 20:23
@janbridley janbridley requested review from a team as code owners May 16, 2024 20:23
@janbridley janbridley requested review from tcmoore3, SchoeniPhlippsn and AlainKadar and removed request for a team May 16, 2024 20:23
@joaander joaander added the validate Execute long running validation tests on pull requests label May 17, 2024
@joaander
Copy link
Member

I think a check against 0 is correct here. ELLIPSOID_OVERLAP_PRECISION is a relative tolerance. If users would like to work in a system of units where typical particle diameters are 1e-50, they we should allow that.

Copy link
Contributor

@SchoeniPhlippsn SchoeniPhlippsn left a comment

Choose a reason for hiding this comment

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

The pull request is straightforward, but we should raise a similar error message for FacetedEllipsoid.

@janbridley
Copy link
Contributor Author

pre-commit.ci autofix

Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

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

Thanks!

@joaander joaander merged commit 6a19c2f into trunk-patch May 29, 2024
40 checks passed
@joaander joaander deleted the fix/#1762-zero-axis-ellipsoid branch May 29, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
validate Execute long running validation tests on pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when any semi-major axis is less than zero in Ellipsoid.
4 participants