Refactor clockwiseOrderedNeighbors() to avoid NaN SEGV from #106 (#108)

* Refactor clockwiseOrderedNeighbors() to avoid NaN SEGV

Although the SEGV when coordinates are NaN has already been
bypassed, I refactored this function when trying to understand
what it did. I find this version more readable, it also happens
to be more efficient (although that probably doesn't matter
with 3-5 substituents).

CoordgenMinimizer.cpp

@@ -1513,11 +1513,11 @@ void CoordgenMinimizer::fallbackOn3DCoordinates(
 bool CoordgenMinimizer::hasNaNCoordinates(
     const std::vector<sketcherMinimizerAtom*>& atoms)
 {
-    for (sketcherMinimizerAtom* a : atoms)
-        if (a->coordinates.x() != a->coordinates.x() ||
-            a->coordinates.y() != a->coordinates.y()) {
+    for (sketcherMinimizerAtom* a : atoms) {
+        if (std::isnan(a->coordinates.x()) || std::isnan(a->coordinates.y())) {
             return true;
         }
+    }
     return false;
 }
 

sketcherMinimizerAtom.cpp

@@ -351,31 +351,24 @@ sketcherMinimizerAtom::expectedValence(unsigned int atomicNumber) const
 vector<sketcherMinimizerAtom*>
 sketcherMinimizerAtom::clockwiseOrderedNeighbors() const
 {
+    vector<pair<float, sketcherMinimizerAtom*>> rankedNeighbors;
+    rankedNeighbors.reserve(neighbors.size());
+    for (auto&& neighbor : neighbors) {
+        float newAngle = sketcherMinimizerMaths::signedAngle(
+                neighbors[0]->coordinates, coordinates,
+                neighbor->coordinates);
+        if (std::isnan(newAngle)) {
+            newAngle = 361;
+        } else if (newAngle < 0) {
+            newAngle += 360;
+        }
+        rankedNeighbors.emplace_back(newAngle, neighbor);
+    }
+    std::sort(rankedNeighbors.begin(), rankedNeighbors.end());
     vector<sketcherMinimizerAtom*> orderedNeighs;
-    vector<sketcherMinimizerAtom*> neighs = neighbors;
-    int lastPoppedIndex = 0;
-    sketcherMinimizerAtom* lastPoppedAtom = neighs[lastPoppedIndex];
-    orderedNeighs.push_back(lastPoppedAtom);
-    neighs.erase(neighs.begin() + lastPoppedIndex);
-
-    while (!neighs.empty()) {
-        float smallestAngle = 361;
-        lastPoppedIndex = 0;
-        for (unsigned int i = 0; i < neighs.size(); i++) {
-            float newAngle = sketcherMinimizerMaths::signedAngle(
-                lastPoppedAtom->coordinates, coordinates,
-                neighs[i]->coordinates);
-            if (newAngle < 0) {
-                newAngle += 360;
-            }
-            if (newAngle < smallestAngle) {
-                smallestAngle = newAngle;
-                lastPoppedIndex = i;
-            }
-        }
-        lastPoppedAtom = neighs[lastPoppedIndex];
-        orderedNeighs.push_back(lastPoppedAtom);
-        neighs.erase(neighs.begin() + lastPoppedIndex);
+    orderedNeighs.reserve(neighbors.size());
+    for (const auto& rankedNeighbor : rankedNeighbors) {
+        orderedNeighs.push_back(rankedNeighbor.second);
     }
     return orderedNeighs;
 }

test/test_coordgen.cpp

@@ -490,6 +490,17 @@ BOOST_AUTO_TEST_CASE(testClockwiseOrderedSubstituents)
     BOOST_REQUIRE_EQUAL(orderedNeighbors[2], neigh2);
 }
 
+BOOST_AUTO_TEST_CASE(testClockwiseOrderedNaN)
+{
+    std::unique_ptr<sketcherMinimizerMolecule> mol("CN(C)C"_smiles);
+    auto& atoms = mol->getAtoms();
+    sketcherMinimizerAtom* center = atoms.at(0);
+    sketcherMinimizerAtom* neigh1 = atoms.at(1);
+    neigh1->coordinates = sketcherMinimizerPointF(std::nanf("name"), std::nanf("name"));
+    const auto orderedNeighbors = center->clockwiseOrderedNeighbors();
+}
+
+
 BOOST_AUTO_TEST_CASE(testbicyclopentane)
 {
     /*