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

Jump to bottom

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 4 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
14 changes: 11 additions & 3 deletions ...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 @@ -399,16 +399,24 @@ public boolean eq(final Vector3D vec, final Precision.DoubleEquivalence precisio

/**
* Get a hashCode for the vector.
* <p>All NaN values have the same hash code.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert formatting. It ad noise the the changes.

*
* <p>
* All NaN values have the same hash code.
* @return a hash code value for this object
*/
@Override
public int hashCode() {
if (isNaN()) {
return 642;
}
return 643 * (164 * Double.hashCode(x) + 3 * Double.hashCode(y) + Double.hashCode(z));
int result = 17;
long temp;
temp = Double.doubleToLongBits(x);
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this was fixed with a different linear combination of Double.hashCode. This seems to have reverted to using doubleToLongBits.

result = (int) (temp ^ (temp >>> 32));
temp = Double.doubleToLongBits(y);
result = 31 * result + (int) (temp ^ (temp >>> 32));
temp = Double.doubleToLongBits(z);
result = 31 * result + (int) (temp ^ (temp >>> 32));
return result;
}

/**
Expand Down
3 changes: 1 addition & 2 deletions ...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 @@ -340,15 +340,14 @@ public boolean eq(final Vector2D vec, final Precision.DoubleEquivalence precisio
* Get a hashCode for the 2D coordinates.
* <p>
* All NaN values have the same hash code.</p>
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert formatting

* @return a hash code value for this object
*/
@Override
public int hashCode() {
if (isNaN()) {
return 542;
}
return 122 * (76 * Double.hashCode(x) + Double.hashCode(y));
return 31 * Double.hashCode(x) + Double.hashCode(y);
}

/**
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
12 changes: 11 additions & 1 deletion ...metry-spherical/src/main/java/org/apache/commons/geometry/spherical/twod/GreatCircle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,17 @@ public boolean eq(final GreatCircle other, final Precision.DoubleEquivalence pre
/** {@inheritDoc} */
@Override
public int hashCode() {
return Objects.hash(pole, u, v, getPrecision());
int result = 1;
long temp;
Copy link
Contributor

Choose a reason for hiding this comment

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

temp does not have to be a long here since all the hashCode() methods will return an int. As such all the xor shift folding is effectively just randomly flipping the bits based on the sign since (temp >>> 32) == 0 for positive or -1 (32-bits set) for negative int hash codes. The xor operation then flips the original int bits if they were negative.

The fact that this works over using Object.hashCode is that some of the hashCodes are negative. So your implementation uses these bit flipped before the addition and the hash is different, but still random.

I think the current implementation is fine. What should be changed is the test. This is assuming that non equal objects will have different hash codes. This is not true. The only contract is that equal objects have equal hash codes.

Since the hash code for the 3-D vectors use the xyx coordinates, using unit vectors results in some of the hash codes being the same. Perhaps try updating the vectors for objects b, c, d to use random xyz coords. The following lines should then pass (unless you are very unlucky with your random vectors).

        // Probably true (unless we are unlucky with the hashes of the vectors)
        Assertions.assertNotEquals(hash, b.hashCode());
        Assertions.assertNotEquals(hash, c.hashCode());
        Assertions.assertNotEquals(hash, d.hashCode());

Copy link
Author

Choose a reason for hiding this comment

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

I think the current implementation is fine. What should be changed is the test.

At present, there is only one assertion causing the test to fail. For these two circles:

final GreatCircle c = GreatCircles.fromPoleAndU(Vector3D.Unit.PLUS_Z, Vector3D.Unit.MINUS_X, TEST_PRECISION);
final GreatCircle a = GreatCircles.fromPoleAndU(Vector3D.Unit.PLUS_Z, Vector3D.Unit.PLUS_X, TEST_PRECISION);

The test currently assumes the hashes must not be equal.

final int hash = a.hashCode();
Assertions.assertNotEquals(hash, c.hashCode());

I might be misunderstanding this point ( please help me clarify ), but it appears that these two circles are topologically identical meaning they occupy the same space.

If this interpretation is correct, we could simply modify the assumption and assert that the hashes of these two circles are expected to be the same.

Assertions.assertEquals(hash, c.hashCode());

With this small adjustment, the test should pass.

temp = pole.hashCode();
result = 31 * result + (int) (temp ^ (temp >>> 32));
temp = u.hashCode();
result = 31 * result + (int) (temp ^ (temp >>> 32));
temp = v.hashCode();
result = 31 * result + (int) (temp ^ (temp >>> 32));
temp = getPrecision().hashCode();
result = 31 * result + (int) (temp ^ (temp >>> 32));
return result;
}

/** {@inheritDoc} */
Expand Down