You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2021/04/07 12:38:25 UTC
[GitHub] [commons-geometry] aherbert commented on a change in pull request #143: GEOMETRY-119: adding Vector.normalizeOrNull() method
aherbert commented on a change in pull request #143:
URL: https://github.com/apache/commons-geometry/pull/143#discussion_r608603343
##########
File path: commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/threed/Vector3D.java
##########
@@ -828,5 +826,88 @@ public Vector3D withNorm(final double mag) {
public Unit negate() {
return new Unit(-getX(), -getY(), -getZ());
}
+
+ /** Create a normalized vector.
+ * @param x Vector coordinate.
+ * @param y Vector coordinate.
+ * @param z Vector coordinate.
+ * @return a vector whose norm is 1.
+ * @throws IllegalArgumentException if the norm of the given value is zero, NaN,
+ * or infinite
+ */
+ public static Unit from(final double x, final double y, final double z) {
+ return tryCreateNormalized(x, y, z, true);
+ }
+
+ /** Create a normalized vector.
+ * @param v Vector.
+ * @return a vector whose norm is 1.
+ * @throws IllegalArgumentException if the norm of the given value is zero, NaN,
+ * or infinite
+ */
+ public static Unit from(final Vector3D v) {
+ return v instanceof Unit ?
+ (Unit) v :
+ from(v.getX(), v.getY(), v.getZ());
+ }
+
+ /** Attempt to create a normalized vector from the given coordinate values. If {@code throwOnFailure}
+ * is true, an exception is thrown if a normalized vector cannot be created. Otherwise, null
+ * is returned.
+ * @param x x coordinate
+ * @param y y coordinate
+ * @param z z coordinate
+ * @param throwOnFailure if true, an exception will be thrown if a normalized vector cannot be created
+ * @return normalized vector or null if one cannot be created and {@code throwOnFailure}
+ * is false
+ * @throws IllegalArgumentException if the computed norm is zero, NaN, or infinite
+ */
+ private static Unit tryCreateNormalized(final double x, final double y, final double z,
+ final boolean throwOnFailure) {
+
+ // Compute the inverse norm directly. If the result is a non-zero real number,
+ // then we can go ahead and construct the unit vector immediately. If not,
+ // we'll do some extra work for edge cases.
+ final double norm = Math.sqrt((x * x) + (y * y) + (z * z));
Review comment:
Handling the common case first is a better solution than checking for scaling immediately.
##########
File path: commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/threed/Vector3D.java
##########
@@ -760,6 +766,21 @@ public static Vector3D linearCombination(final double a1, final Vector3D v1,
/** Negation of unit vector (coordinates: 0, 0, -1). */
public static final Unit MINUS_Z = new Unit(0d, 0d, -1d);
+ /** Maximum coordinate value for computing normalized vectors
+ * with raw, unscaled values.
+ */
+ private static final double UNSCALED_MAX = 0x1.0p+500;
+
+ /** Factor used to scale up coordinate values in order to produce
+ * normalized coordinates without overflow or underflow.
+ */
+ private static final double SCALE_UP_FACTOR = 0x1.0p600;
Review comment:
For consistency use a '+' here before the 600 or drop it from the UNSCALED_MAX constant.
##########
File path: commons-geometry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/threed/Vector3DTest.java
##########
@@ -396,16 +395,40 @@ public void testNormalize() {
checkVector(Vector3D.of(2, 2, 2).normalize(), invSqrt3, invSqrt3, invSqrt3);
checkVector(Vector3D.of(-2, -2, -2).normalize(), -invSqrt3, -invSqrt3, -invSqrt3);
+ checkVector(Vector3D.of(Double.MIN_VALUE, 0, 0).normalize(), 1, 0, 0);
+ checkVector(Vector3D.of(0, Double.MIN_VALUE, 0).normalize(), 0, 1, 0);
+ checkVector(Vector3D.of(0, 0, Double.MIN_VALUE).normalize(), 0, 0, 1);
+
+ checkVector(Vector3D.of(Double.MIN_VALUE, Double.MIN_VALUE, Double.MIN_VALUE).normalize(),
+ invSqrt3, invSqrt3, invSqrt3);
+
+ checkVector(Vector3D.of(Double.MIN_NORMAL, 0, 0).normalize(), 1, 0, 0);
+ checkVector(Vector3D.of(0, Double.MIN_NORMAL, 0).normalize(), 0, 1, 0);
+ checkVector(Vector3D.of(0, 0, Double.MIN_NORMAL).normalize(), 0, 0, 1);
+
+ checkVector(Vector3D.of(Double.MIN_NORMAL, Double.MIN_NORMAL, Double.MIN_NORMAL).normalize(),
Review comment:
Use a negative here for one of the values (as done using MAX_VALUE below).
##########
File path: commons-geometry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/threed/Vector3DTest.java
##########
@@ -419,6 +442,42 @@ public void testNormalize_isIdempotent() {
checkVector(v.normalize(), invSqrt3, invSqrt3, invSqrt3);
}
+ @Test
+ public void testNormalizeOrNull() {
+ // arrange
+ final double invSqrt3 = 1 / Math.sqrt(3);
+
+ // act/assert
+ checkVector(Vector3D.of(100, 0, 0).normalizeOrNull(), 1, 0, 0);
+ checkVector(Vector3D.of(-100, 0, 0).normalizeOrNull(), -1, 0, 0);
+
+ checkVector(Vector3D.of(2, 2, 2).normalizeOrNull(), invSqrt3, invSqrt3, invSqrt3);
+ checkVector(Vector3D.of(-2, -2, -2).normalizeOrNull(), -invSqrt3, -invSqrt3, -invSqrt3);
+
+ checkVector(Vector3D.of(Double.MIN_VALUE, 0, 0).normalizeOrNull(), 1, 0, 0);
+ checkVector(Vector3D.of(0, Double.MIN_VALUE, 0).normalizeOrNull(), 0, 1, 0);
+ checkVector(Vector3D.of(0, 0, Double.MIN_VALUE).normalizeOrNull(), 0, 0, 1);
+
+ checkVector(Vector3D.of(-Double.MAX_VALUE, -Double.MAX_VALUE, -Double.MAX_VALUE).normalizeOrNull(),
Review comment:
There is no test using three MIN_NORMAL coordinates as is done in the testNormalise test
##########
File path: commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/twod/Vector2D.java
##########
@@ -686,6 +692,21 @@ public static Vector2D linearCombination(final double a1, final Vector2D v1,
/** Negation of unit vector (coordinates: 0, -1). */
public static final Unit MINUS_Y = new Unit(0d, -1d);
+ /** Maximum coordinate value for computing normalized vectors
+ * with raw, unscaled values.
+ */
+ private static final double UNSCALED_MAX = 0x1.0p+500;
+
+ /** Factor used to scale up coordinate values in order to produce
+ * normalized coordinates without overflow or underflow.
+ */
+ private static final double SCALE_UP_FACTOR = 0x1.0p600;
Review comment:
Update constants as noted before
##########
File path: commons-geometry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/twod/Vector2DTest.java
##########
@@ -317,21 +317,43 @@ public void testSubtract_scaled() {
@Test
public void testNormalize() {
+ // arrange
+ final double invSqrt2 = 1.0 / Math.sqrt(2);
+
// act/assert
checkVector(Vector2D.of(100, 0).normalize(), 1, 0);
checkVector(Vector2D.of(-100, 0).normalize(), -1, 0);
checkVector(Vector2D.of(0, 100).normalize(), 0, 1);
checkVector(Vector2D.of(0, -100).normalize(), 0, -1);
checkVector(Vector2D.of(-1, 2).normalize(), -1.0 / Math.sqrt(5), 2.0 / Math.sqrt(5));
+
+ checkVector(Vector2D.of(Double.MIN_VALUE, 0).normalize(), 1, 0);
+ checkVector(Vector2D.of(0, Double.MIN_VALUE).normalize(), 0, 1);
+
+ checkVector(Vector2D.of(Double.MIN_VALUE, Double.MIN_VALUE).normalize(), invSqrt2, invSqrt2);
+
+ checkVector(Vector2D.of(Double.MIN_NORMAL, 0).normalize(), 1, 0, 0);
+ checkVector(Vector2D.of(0, Double.MIN_NORMAL).normalize(), 0, 1, 0);
+
+ checkVector(Vector2D.of(Double.MIN_NORMAL, Double.MIN_NORMAL).normalize(), invSqrt2, invSqrt2);
Review comment:
Use a negative for one of the values.
##########
File path: commons-geometry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/twod/Vector2DTest.java
##########
@@ -345,6 +367,40 @@ public void testNormalize_isIdempotent() {
checkVector(v.normalize(), invSqrt2, invSqrt2);
}
+ @Test
+ public void testNormalizeOrNull() {
+ // arrange
+ final double invSqrt2 = 1 / Math.sqrt(2);
+
+ // act/assert
+ checkVector(Vector2D.of(100, 0).normalizeOrNull(), 1, 0);
+ checkVector(Vector2D.of(-100, 0).normalizeOrNull(), -1, 0);
+
+ checkVector(Vector2D.of(2, 2).normalizeOrNull(), invSqrt2, invSqrt2);
+ checkVector(Vector2D.of(-2, -2).normalizeOrNull(), -invSqrt2, -invSqrt2);
+
+ checkVector(Vector2D.of(Double.MIN_VALUE, 0).normalizeOrNull(), 1, 0);
+ checkVector(Vector2D.of(0, Double.MIN_VALUE).normalizeOrNull(), 0, 1);
+
+ checkVector(Vector2D.of(Double.MAX_VALUE, -Double.MAX_VALUE).normalizeOrNull(), invSqrt2, -invSqrt2);
Review comment:
There is no test using two MIN_NORMAL coordinates as is done in the testNormalise test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org