kinow commented on a change in pull request #194: URL: https://github.com/apache/commons-geometry/pull/194#discussion_r825373065
########## File path: commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/PointMap3DImpl.java ########## @@ -0,0 +1,174 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.geometry.euclidean; + +import java.util.List; + +import org.apache.commons.geometry.core.collection.PointMap; +import org.apache.commons.geometry.core.internal.AbstractBucketPointMap; +import org.apache.commons.geometry.euclidean.threed.Vector3D; +import org.apache.commons.numbers.core.Precision; + +/** Internal {@link PointMap3D} implementation. + * @param <V> Map value type + */ +final class PointMap3DImpl<V> + extends AbstractBucketPointMap<Vector3D, V> + implements PointMap<Vector3D, V> { + + /** Number of children per node. */ + private static final int NODE_CHILD_COUNT = 8; + + /** Max entries per node. This value was determined empirically was chosen to + * provide a balance between having a small number of entries in each node when + * searching and having a large number of samples to provide a good split point + * during insertion. Review comment: Maybe link to the JMH tests, or just point the reader to the JIRA number (I remember reading the comments where you detailed your findings trying different algorithms.) This could save someone curious about this statement some time, I think. ########## File path: commons-geometry-core/src/main/java/org/apache/commons/geometry/core/internal/PointMapAsSetAdapter.java ########## @@ -0,0 +1,89 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.geometry.core.internal; + +import java.util.AbstractSet; +import java.util.Iterator; + +import org.apache.commons.geometry.core.Point; +import org.apache.commons.geometry.core.collection.PointMap; +import org.apache.commons.geometry.core.collection.PointSet; + +/** Class that exposes a {@link PointMap} as a {@link PointSet}. + * @param <P> Point type + * @param <M> Map type + */ +public class PointMapAsSetAdapter<P extends Point<P>, M extends PointMap<P, Object>> + extends AbstractSet<P> + implements PointSet<P> { + + /** Dummy map value used to indicate presence in the set. */ + private static final Object PRESENT = new Object(); + + /** Backing map. */ + private final M map; + + /** Construct a new instance that use the argument as its backing map. + * @param backingMap backing map + */ + public PointMapAsSetAdapter(final M backingMap) { + this.map = backingMap; + } + + /** {@inheritDoc} */ + @Override + public P resolve(final P pt) { + return map.resolveKey(pt); + } + + /** {@inheritDoc} */ + @Override + public Iterator<P> iterator() { + return map.keySet().iterator(); + } + + /** {@inheritDoc} */ + @Override + public int size() { + return map.size(); + } + + /** {@inheritDoc} */ + @Override + public boolean contains(final Object obj) { + return map.containsKey(obj); + } + + /** {@inheritDoc} */ + @Override + public boolean add(final P pt) { + return map.put(pt, PRESENT) == null; + } + + /** {@inheritDoc} */ + @Override + public boolean remove(final Object obj) { + final Object prev = map.remove(obj); + return GeometryInternalUtils.sameInstance(prev, PRESENT); Review comment: BTW, I've created a gist with the test files I'm using (I had older files on my working copy, that I accidentally deleted after re-syncing the branches :sweat_smile:, so saving it there in case I'm on another computer or deleted them again ): https://gist.github.com/kinow/c590789efba00d4449eb1f5e5a7047ec ########## File path: commons-geometry-core/src/main/java/org/apache/commons/geometry/core/internal/PointMapAsSetAdapter.java ########## @@ -0,0 +1,89 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.geometry.core.internal; + +import java.util.AbstractSet; +import java.util.Iterator; + +import org.apache.commons.geometry.core.Point; +import org.apache.commons.geometry.core.collection.PointMap; +import org.apache.commons.geometry.core.collection.PointSet; + +/** Class that exposes a {@link PointMap} as a {@link PointSet}. + * @param <P> Point type + * @param <M> Map type + */ +public class PointMapAsSetAdapter<P extends Point<P>, M extends PointMap<P, Object>> + extends AbstractSet<P> + implements PointSet<P> { + + /** Dummy map value used to indicate presence in the set. */ + private static final Object PRESENT = new Object(); + + /** Backing map. */ + private final M map; + + /** Construct a new instance that use the argument as its backing map. + * @param backingMap backing map + */ + public PointMapAsSetAdapter(final M backingMap) { + this.map = backingMap; + } + + /** {@inheritDoc} */ + @Override + public P resolve(final P pt) { + return map.resolveKey(pt); + } + + /** {@inheritDoc} */ + @Override + public Iterator<P> iterator() { + return map.keySet().iterator(); + } + + /** {@inheritDoc} */ + @Override + public int size() { + return map.size(); + } + + /** {@inheritDoc} */ + @Override + public boolean contains(final Object obj) { + return map.containsKey(obj); + } + + /** {@inheritDoc} */ + @Override + public boolean add(final P pt) { + return map.put(pt, PRESENT) == null; + } + + /** {@inheritDoc} */ + @Override + public boolean remove(final Object obj) { + final Object prev = map.remove(obj); + return GeometryInternalUtils.sameInstance(prev, PRESENT); Review comment: @darkma773r I think this can cause some issues, so we should either change it or document it for users. For example, if I have ```java Vector1D point = Vector1D.of(10d); PointMap<Vector1D, Object> map = EuclideanCollections.pointMap1D(Precision.doubleEquivalenceOfEpsilon(0.01d)); map.put(point, 10_000); ``` and create a `set` with this adapter, ```java PointMapAsSetAdapter<Vector1D, PointMap<Vector1D, Object>> fakeSet = new PointMapAsSetAdapter<Vector1D, PointMap<Vector1D, Object>>(map); ``` then if I try to remove the point that was previously put into the map, this method will return `false`, as it only returns `true` — IIUC — for entries added using the `add` method of the set created by the adapter (which uses the `INSTANCE` member.) ``` boolean b = fakeSet.remove(point); System.out.println(b); // false ``` Finally, if I add the point already used in the map, again to the `set` ```java fakeSet.add(point); ``` Now `remove` would return `true`, as the `add` replaces the previously created entry. I know the `PointSet` interface documents that `PointSet` is not strictly a `Set`, but **I think** we are using `@inheritDocs` here which will bring the docs from `Set`? … if so I think we would need to document the behavior of these methods now (tedious/boring but I think needed?). ########## File path: commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/PointMap3DImpl.java ########## @@ -0,0 +1,174 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.geometry.euclidean; + +import java.util.List; + +import org.apache.commons.geometry.core.collection.PointMap; +import org.apache.commons.geometry.core.internal.AbstractBucketPointMap; +import org.apache.commons.geometry.euclidean.threed.Vector3D; +import org.apache.commons.numbers.core.Precision; + +/** Internal {@link PointMap3D} implementation. + * @param <V> Map value type + */ +final class PointMap3DImpl<V> + extends AbstractBucketPointMap<Vector3D, V> + implements PointMap<Vector3D, V> { + + /** Number of children per node. */ + private static final int NODE_CHILD_COUNT = 8; + + /** Max entries per node. This value was determined empirically was chosen to Review comment: >was determined empirically was chosen and was chosen? ########## File path: commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/PointMap3DImpl.java ########## @@ -0,0 +1,174 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.geometry.euclidean; + +import java.util.List; + +import org.apache.commons.geometry.core.collection.PointMap; +import org.apache.commons.geometry.core.internal.AbstractBucketPointMap; +import org.apache.commons.geometry.euclidean.threed.Vector3D; +import org.apache.commons.numbers.core.Precision; + +/** Internal {@link PointMap3D} implementation. + * @param <V> Map value type + */ +final class PointMap3DImpl<V> + extends AbstractBucketPointMap<Vector3D, V> + implements PointMap<Vector3D, V> { + + /** Number of children per node. */ + private static final int NODE_CHILD_COUNT = 8; + + /** Max entries per node. This value was determined empirically was chosen to + * provide a balance between having a small number of entries in each node when + * searching and having a large number of samples to provide a good split point + * during insertion. + */ + private static final int MAX_ENTRIES_PER_NODE = 32; + + /** X negative octant flag. */ + private static final int XNEG = 1 << 5; + + /** X postive octant flag. */ + private static final int XPOS = 1 << 4; + + /** Y negative octant flag. */ + private static final int YNEG = 1 << 3; + + /** Y positive octant flag. */ + private static final int YPOS = 1 << 2; + + /** Z negative octant flag. */ + private static final int ZNEG = 1 << 1; + + /** Z positive octant flag. */ + private static final int ZPOS = 1; + + /** Octant location flags for child nodes. */ + private static final int[] CHILD_LOCATIONS = { + XNEG | YNEG | ZNEG, + XNEG | YNEG | ZPOS, + XNEG | YPOS | ZNEG, + XNEG | YPOS | ZPOS, + + XPOS | YNEG | ZNEG, + XPOS | YNEG | ZPOS, + XPOS | YPOS | ZNEG, + XPOS | YPOS | ZPOS + }; + + /** Construct a new instance using the given precision context to determine + * floating point equality. + * @param precision precision context + */ + PointMap3DImpl(final Precision.DoubleEquivalence precision) { + super(MapNode3D::new, Review comment: Ha! I remember you showing me this trick in that Commons Imaging PR. Good to see that again to fixate that pattern on my :brain: :+1: ########## File path: commons-geometry-examples/examples-jmh/src/main/java/org/apache/commons/geometry/examples/jmh/euclidean/pointmap/RebuildingKDTree.java ########## @@ -0,0 +1,259 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.geometry.examples.jmh.euclidean.pointmap; + +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; + +import org.apache.commons.geometry.euclidean.threed.Vector3D; +import org.apache.commons.numbers.core.Precision.DoubleEquivalence; + +/** {@link KDTree} subclass that rebuilds the tree periodically in an + * attempt to restrict the overall height of the tree. + * @param <V> Value type + */ +public class RebuildingKDTree<V> extends KDTree<V> { + + /** Default rebuild maximum. */ + private static final int DEFAULT_REBUILD_MAX = 16; + + /** Maximum size of the tree before it is rebuilt. */ + private int rebuildMax = DEFAULT_REBUILD_MAX; + + /** Minimum size of the tree before it is rebuilt. */ + private int rebuildMin; + + /** Construct a new instance. + * @param precision precision context + */ + public RebuildingKDTree(final DoubleEquivalence precision) { + super(precision); + } + + /** {@inheritDoc} */ + @Override + public V put(final Vector3D key, final V value) { + final V result = super.put(key, value); + + if (size() >= rebuildMax) { + rebuild(); + } + + return result; + } + + /** {@inheritDoc} */ + @Override + public V remove(final Object key) { + final V result = super.remove(key); + + if (size() <= rebuildMin) { + rebuild(); + } + + return result; + } + + /** + * Rebuild the tree, attempting to reduce the tree depth. + */ + public void rebuild() { + int n = size(); + if (n > 0) { + // construct an array list containing all of the tree nodes + final List<KDTreeNode<V>> nodes = collectNodes(); + + // rebuild recursively and set the new root + final KDTreeNode<V> newRoot = rebuildRecursive(nodes, 0, n, 0); + + setRoot(newRoot); + } + + rebuildMax = Math.max(DEFAULT_REBUILD_MAX, 2 * n); + rebuildMin = n / 2; + } + + /** Get a list containing all nodes in the tree. The node connections are all cleared. + * @return list containing all nodes in the tree + */ + protected List<KDTreeNode<V>> collectNodes() { + final List<KDTreeNode<V>> nodes = new ArrayList<>(size()); + collectNodesRecursive(getRoot(), nodes); + + return nodes; + } + + /** Add nodes in the subtree rooted at {@code curr} to {@code nodes}. The node connection + * references are all cleared. + * @param curr subtree root node + * @param nodes node list + */ + protected void collectNodesRecursive(final KDTreeNode<V> curr, final List<KDTreeNode<V>> nodes) { + if (curr != null) { + collectNodesRecursive(curr.getLeft(), nodes); + nodes.add(curr); + collectNodesRecursive(curr.getRight(), nodes); + + curr.setParent(null); + curr.setLeft(null); + curr.setRight(null); + } + } + + /** Recursively rebuild the tree using the specified node sublist. + * @param nodes node list + * @param startIdx sub list start index (inclusive) + * @param endIdx sub list end index (exclusive) + * @param depth node depth + * @return the root of the subtree containing the nodes between {@code startIdx} and {@code endIdx} + */ + protected KDTreeNode<V> rebuildRecursive(final List<KDTreeNode<V>> nodes, final int startIdx, final int endIdx, + final int depth) { + final CutDimension cutDimension = getCutDimensionForDepth(depth); + + final KDTreeNode<V> node; + if ((endIdx - startIdx) < 2) { + // only a single node here + node = nodes.get(startIdx); + } else { + final int splitIdx = partition(nodes, startIdx, endIdx, cutDimension); + + node = nodes.get(splitIdx); + + if (startIdx < splitIdx) { + node.setLeft(rebuildRecursive(nodes, startIdx, splitIdx, depth + 1)); + node.getLeft().setParent(node); + } + + if (splitIdx < endIdx - 1) { + node.setRight(rebuildRecursive(nodes, splitIdx + 1, endIdx, depth + 1)); + node.getRight().setParent(node); + } + } + + node.setCutDimension(cutDimension); + + return node; + } + + /** Partition the given sublist into values below the median and values above. The + * index of the median is returned. + * @param nodes node list + * @param startIdx start index (inclusive) of the sublist to partition + * @param endIdx end index (exclusive) of the sublist to partition + * @param cutDimension cut dimension + * @return index of the sublist median + */ + protected int partition(final List<KDTreeNode<V>> nodes, final int startIdx, final int endIdx, + final CutDimension cutDimension) { + int n = endIdx - startIdx; + if (n < 2) { + return startIdx; + } else if (n == 2) { + final int bIdx = endIdx - 1; + + final double a = cutDimension.getCoordinate(nodes.get(startIdx).getKey()); + final double b = cutDimension.getCoordinate(nodes.get(bIdx).getKey()); + if (a <= b) { + return startIdx; + } else { + swap(nodes, startIdx, bIdx); + return bIdx; + } + } else { + return findMedianStart(nodes, startIdx, endIdx, cutDimension); + } + } + + /** Find the starting index of the node median value in the given list. The list is + * partially sorted and value less than the median come before the returned index while + * values greater than or equal come after. + * @param nodes list of node + * @param startIdx sublist start index (inclusive) + * @param endIdx sublist end index (exclusive) + * @param cutDimension cut dimension + * @return index of the median in the specific sublist of {@code nodes} along the cut dimension + */ + protected int findMedianStart(final List<KDTreeNode<V>> nodes, final int startIdx, final int endIdx, + final CutDimension cutDimension) { + int k = startIdx + ((endIdx - startIdx) / 2); + int low = startIdx; + int high = endIdx - 1; + int lowTemp; + int highTemp; + double x; + while (low < high) { + x = cutDimension.getCoordinate(nodes.get(k).getKey()); + lowTemp = low; + highTemp = high; + do { + while (cutDimension.getCoordinate(nodes.get(lowTemp).getKey()) < x) { + ++lowTemp; + } + while (cutDimension.getCoordinate(nodes.get(highTemp).getKey()) > x) { + --highTemp; + } + + if (lowTemp <= highTemp) { + swap(nodes, lowTemp, highTemp); + + ++lowTemp; + --highTemp; + } + } while (lowTemp <= highTemp); + + if (k < lowTemp) { + // search low part + high = highTemp; + } + if (k > highTemp) { + // search high part + low = lowTemp; + } + } + + // back up to the start of the median value + x = cutDimension.getCoordinate(nodes.get(k).getKey()); + while (k > startIdx && + cutDimension.getCoordinate(nodes.get(k - 1).getKey()) == x) { + --k; + } + + return k; + } + + /** Construct a comparator the sorts by the given cut dimension. + * @param cutDimension cut dimension to sort by + * @return comparator along the cut dimension + */ + protected Comparator<KDTreeNode<V>> comparator(final CutDimension cutDimension) { + return (a, b) -> Double.compare(cutDimension.getCoordinate(a.getKey()), cutDimension.getCoordinate(b.getKey())); + } + + /** Swap two elements in {@code list}. + * @param <T> List element type + * @param list input list + * @param aIdx index of the first element + * @param bIdx index of the second element + */ + private static <T> void swap(final List<T> list, final int aIdx, final int bIdx) { + final T temp = list.get(aIdx); + list.set(aIdx, list.get(bIdx)); + list.set(bIdx, temp); Review comment: Couldn't we use `Collections.swap(list, aIdx, bIdx);` here? ########## File path: commons-geometry-spherical/src/main/java/org/apache/commons/geometry/spherical/PointMap2SImpl.java ########## @@ -0,0 +1,181 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.geometry.spherical; + +import java.util.List; + +import org.apache.commons.geometry.core.collection.PointMap; +import org.apache.commons.geometry.core.internal.AbstractBucketPointMap; +import org.apache.commons.geometry.core.partitioning.HyperplaneLocation; +import org.apache.commons.geometry.euclidean.threed.Vector3D; +import org.apache.commons.geometry.spherical.twod.GreatCircle; +import org.apache.commons.geometry.spherical.twod.GreatCircles; +import org.apache.commons.geometry.spherical.twod.Point2S; +import org.apache.commons.numbers.core.Precision; + +/** Internal implementation of {@link PointMap2S}. + * @param <V> Map value type + */ +final class PointMap2SImpl<V> + extends AbstractBucketPointMap<Point2S, V> + implements PointMap<Point2S, V> { + + /** Number of children per node. */ + private static final int NODE_CHILD_COUNT = 4; + + /** Max entries per node. */ + private static final int MAX_ENTRIES_PER_NODE = 16; + + /** First negative quadrant flag. */ + private static final int NEG1 = 1 << 3; + + /** First positive quadrant flag. */ + private static final int POS1 = 1 << 2; + + /** Second negative quadrant flag. */ + private static final int NEG2 = 1 << 1; + + /** Second positive quadrant flag. */ + private static final int POS2 = 1; + + /** Location flags for child nodes. */ + private static final int[] CHILD_LOCATIONS = { + NEG1 | NEG2, + NEG1 | POS2, + POS1 | NEG2, + POS1 | POS2 + }; + + PointMap2SImpl(final Precision.DoubleEquivalence precision) { + super(MapNode2S::new, + MAX_ENTRIES_PER_NODE, + NODE_CHILD_COUNT, + precision); + } + + /** {@inheritDoc} */ + @Override + protected boolean pointsEq(final Point2S a, final Point2S b) { + return a.eq(b, getPrecision()); + } + + /** Tree node class for {@link PointMap2SImpl}. + * @param <V> Map value type + */ + private static final class MapNode2S<V> + extends BucketNode<Point2S, V> { + + /** First hyperplane split. */ + private GreatCircle firstSplit; + + /** Second hyperplane split. */ + private GreatCircle secondSplit; + + MapNode2S( + final AbstractBucketPointMap<Point2S, V> map, + final BucketNode<Point2S, V> parent) { + super(map, parent); + } + + /** {@inheritDoc} */ + @Override + protected void computeSplit() { + final Vector3D.Sum sum = Vector3D.Sum.create(); + + for (Entry<Point2S, V> entry : this) { + sum.add(entry.getKey().getVector()); + } + + // construct an orthonormal basis + Vector3D.Unit u = sum.get().multiply(1.0 / MAX_ENTRIES_PER_NODE) + .normalizeOrNull(); + if (u == null) { + u = Vector3D.Unit.PLUS_X; + } + + Vector3D.Unit v = Vector3D.Unit.PLUS_Z.cross(u) + .normalizeOrNull(); + if (v == null) { + v = Vector3D.Unit.PLUS_Y.cross(u) + .normalizeOrNull(); + } + final Vector3D.Unit w = u.cross(v).normalize(); + + // construct the two great circles + firstSplit = GreatCircles.fromPole(v.add(w), getPrecision()); + secondSplit = GreatCircles.fromPole(v.negate().add(w), getPrecision()); Review comment: Had to google orthonormality to remind me of the concept. Wish I had more time to dive deeper into the code & concepts here, and really understand everything that's going on here :nerd_face: :neckbeard: but looking at the code everything looks OK IMO :+1: (it's fun to be able to read code like this, one of the benefits of participating in ASF & Commons development) -- 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. To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
