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.

Sign up for GitHub

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

Fix hash code collision for Vector2D and Vector3D #227

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix hash code collision for Vector2D and Vector3D #227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
93a01fa
Fix hash code collision for Vector2D and Vector3D
ivanshuba Jan 16, 2024
d313086
Clarify comments describing hashing algorithm
ivanshuba Jan 17, 2024
ddb75f0
Make the Vector2D and Vector3D hashCode method more readable
ivanshuba Jan 17, 2024
448c2d3
Modify GreatCircle hashCode calculation
ivanshuba Jan 18, 2024
0341398
Revert GreatCircle hash code calculation
ivanshuba Jan 30, 2024
39c7ef0
Add changes requested in PR review
ivanshuba Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ...ometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/threed/Vector3D.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ public Vector3D reject(final Vector3D base) {
* frame with one of the axes in a predefined direction. The
* following example shows how to build a frame having the k axis
* aligned with the known vector u :
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Undo this formatting.

* <pre><code>
* Vector3D k = u.normalize();
* Vector3D i = k.orthogonal();
Expand Down Expand Up @@ -408,7 +409,11 @@ public int hashCode() {
if (isNaN()) {
return 642;
}
return 643 * (164 * Double.hashCode(x) + 3 * Double.hashCode(y) + Double.hashCode(z));
int result = 1;
result = 31 * result + Double.hashCode(x);
result = 31 * result + Double.hashCode(y);
result = 31 * result + Double.hashCode(z);
return result;
}

/**
Expand Down
5 changes: 4 additions & 1 deletion ...geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/twod/Vector2D.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,10 @@ public int hashCode() {
if (isNaN()) {
return 542;
}
return 122 * (76 * Double.hashCode(x) + Double.hashCode(y));
int result = 1;
result = 31 * result + Double.hashCode(x);
result = 31 * result + Double.hashCode(y);
return result;
}

/**
Expand Down
60 changes: 59 additions & 1 deletion ...ry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/threed/Vector3DTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import java.util.Comparator;
import java.util.List;
import java.util.regex.Pattern;

Copy link
Contributor

Choose a reason for hiding this comment

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

Undo this formatting.

import org.apache.commons.geometry.core.GeometryTestUtils;
import org.apache.commons.geometry.euclidean.EuclideanTestUtils;
import org.apache.commons.numbers.angle.Angle;
Expand Down Expand Up @@ -1146,6 +1145,65 @@ void testEqualsAndHashCode_signedZeroConsistency() {
Assertions.assertEquals(b.hashCode(), d.hashCode());
}

@Test
void testHashCodeCollisions_symmetricAboutZero() {
final double any = Math.random();
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this could create sporadic failures. We can formalise this using some random numbers:

@ParameterizedTest
@ValueSource(doubles = {1.23, 4.56, 42, Math.PI})
void testHashCodeCollisions_symmetricAboutZero(double any) {

final double neg = -any;
final double pos = +any;

final Vector3D negX = Vector3D.of(neg, 0.0, 0.0);
final Vector3D posX = Vector3D.of(pos, 0.0, 0.0);
final Vector3D negY = Vector3D.of(0.0, neg, 0.0);
final Vector3D posY = Vector3D.of(0.0, pos, 0.0);
final Vector3D negZ = Vector3D.of(0.0, 0.0, neg);
final Vector3D posZ = Vector3D.of(0.0, 0.0, pos);

int xNegHash = negX.hashCode();
int xPosHash = posX.hashCode();
int yNegHash = negY.hashCode();
int yPosHash = posY.hashCode();
int zNegHash = negZ.hashCode();
int zPosHash = posZ.hashCode();

String xMessage = String.format("negXHash: %s, posXHash: %s (expected to be non-equal)\n", xNegHash, xPosHash);
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be passed a suppliers to Assertions. However the Assertions will already have a statement containing the two values and that they were expected to be not equal. So we can just use:

Assertions.assertNotEquals(zNegHash, zPosHash, "+/- z hash");

String yMessage = String.format("negYHash: %s, posYHash: %s (expected to be non-equal)\n", yNegHash, yPosHash);
String zMessage = String.format("negZHash: %s, posZHash: %s (expected to be non-equal)\n", zNegHash, zPosHash);

Assertions.assertNotEquals(zNegHash, zPosHash, zMessage);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these in z, y, x order? Makes more sense to use x, y, z, order.

Assertions.assertNotEquals(yNegHash, yPosHash, yMessage);
Assertions.assertNotEquals(xNegHash, xPosHash, xMessage);
}

@Test
void testHashCodeCollisions_symmetricAboutArbitraryValue() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as testHashCodeCollisions_symmetricAboutZero. Use a ParameterizedTest and simplify the assertion message.

@ParameterizedTest
@CsvSource(
   "1.23, 72.68",
   // etc
)
void testHashCodeCollisions_symmetricAboutArbitraryValue(double any, double arb) {

final double any = Math.random();
final double arb = Math.random();
final double neg = -any;
final double pos = +any;

final Vector3D negX = Vector3D.of(neg, arb, arb);
final Vector3D posX = Vector3D.of(pos, arb, arb);
final Vector3D negY = Vector3D.of(arb, neg, arb);
final Vector3D posY = Vector3D.of(arb, pos, arb);
final Vector3D negZ = Vector3D.of(arb, arb, neg);
final Vector3D posZ = Vector3D.of(arb, arb, pos);

int xNegHash = negX.hashCode();
int xPosHash = posX.hashCode();
int yNegHash = negY.hashCode();
int yPosHash = posY.hashCode();
int zNegHash = negZ.hashCode();
int zPosHash = posZ.hashCode();

String xMessage = String.format("negXHash: %s, posXHash: %s (expected to be non-equal)\n", xNegHash, xPosHash);
String yMessage = String.format("negYHash: %s, posYHash: %s (expected to be non-equal)\n", yNegHash, yPosHash);
String zMessage = String.format("negZHash: %s, posZHash: %s (expected to be non-equal)\n", zNegHash, zPosHash);

Assertions.assertNotEquals(zNegHash, zPosHash, zMessage);
Assertions.assertNotEquals(yNegHash, yPosHash, yMessage);
Assertions.assertNotEquals(xNegHash, xPosHash, xMessage);
}

@Test
void testToString() {
// arrange
Expand Down
48 changes: 47 additions & 1 deletion ...etry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/twod/Vector2DTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,6 @@ void testAngle() {
Assertions.assertEquals(0.004999958333958323, Vector2D.of(20.0, 0.0).angle(Vector2D.of(20.0, 0.1)), EPS);
}


@Test
void testAngle_illegalNorm() {
// arrange
Expand Down Expand Up @@ -912,6 +911,53 @@ void testHashCode() {
Assertions.assertEquals(Vector2D.of(0, Double.NaN).hashCode(), Vector2D.of(Double.NaN, 0).hashCode());
}

@Test
void testHashCodeCollisions_symmetricAboutZero() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as previous. Use a ParameterizedTest and simplify the assertion message.

final double any = Math.random();
final double neg = -any;
final double pos = +any;

final Vector2D negX = Vector2D.of(neg, 0.0);
final Vector2D posX = Vector2D.of(pos, 0.0);
final Vector2D negY = Vector2D.of(0.0, neg);
final Vector2D posY = Vector2D.of(0.0, pos);

int xNegHash = negX.hashCode();
int xPosHash = posX.hashCode();
int yNegHash = negY.hashCode();
int yPosHash = posY.hashCode();

String xMessage = String.format("xNegHash: %s, xPosHash: %s (expected to be non-equal)\n", xNegHash, xPosHash);
String yMessage = String.format("yNegHash: %s, yPosHash: %s (expected to be non-equal)\n", yNegHash, yPosHash);

Assertions.assertNotEquals(xNegHash, xPosHash, xMessage);
Assertions.assertNotEquals(yNegHash, yPosHash, yMessage);
}

@Test
void testHashCodeCollisions_symmetricAboutArbitraryValue() {
final double any = Math.random();
final double arb = Math.random();
final double neg = -any;
final double pos = +any;

final Vector2D negX = Vector2D.of(neg, arb);
final Vector2D posX = Vector2D.of(pos, arb);
final Vector2D negY = Vector2D.of(arb, neg);
final Vector2D posY = Vector2D.of(arb, pos);

int xNegHash = negX.hashCode();
int xPosHash = posX.hashCode();
int yNegHash = negY.hashCode();
int yPosHash = posY.hashCode();

String xMessage = String.format("xNegHash: %s, xPosHash: %s (expected to be non-equal)\n", xNegHash, xPosHash);
String yMessage = String.format("yNegHash: %s, yPosHash: %s (expected to be non-equal)\n", yNegHash, yPosHash);

Assertions.assertNotEquals(xNegHash, xPosHash, xMessage);
Assertions.assertNotEquals(yNegHash, yPosHash, yMessage);
}

@Test
void testEquals() {
// arrange
Expand Down
2 changes: 1 addition & 1 deletion ...y-spherical/src/test/java/org/apache/commons/geometry/spherical/twod/GreatCircleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,7 +664,7 @@ void testHashCode() {
Assertions.assertEquals(hash, a.hashCode());

Assertions.assertNotEquals(hash, b.hashCode());
Assertions.assertNotEquals(hash, c.hashCode());
Assertions.assertEquals(hash, c.hashCode());
Copy link
Contributor

Choose a reason for hiding this comment

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

If the two circles are the same then this should be tested using Assertions.assertEquals. If they are the same then the hash code must be the same. If the objects are not equal then the hash code may or may not collide; it doesn't matter.

Any test that uses not equals on hash codes for objects that are not equal can potentially fail due to random hash collision.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is testing the hash codes are equal (because the objects are equal) then we should test the objects are equal:

// Equal objects have equal hash codes
Assertions.assertEquals(a, c);
Assertions.assertEquals(hash, c.hashCode());

Assertions.assertNotEquals(hash, d.hashCode());

Assertions.assertEquals(hash, e.hashCode());
Expand Down