From b64da963d44db7e95962cb2f5ead5c4822e773e0 Mon Sep 17 00:00:00 2001
From: lisa <97008179+lisamessier@users.noreply.github.com>
Date: Fri, 27 Sep 2024 11:50:40 -0400
Subject: [PATCH] Handle negative arguments in most_rational (#1269)

Reorder logic in `grid.most_rational` to handle negative arguments.

---------

Co-authored-by: Rahul Gaur <19224702+rahulgaur104@users.noreply.github.com>
Co-authored-by: Rahul <rgaur@terpmail.umd.edu>
---
 desc/grid.py       | 20 +++++++++++---------
 tests/test_grid.py | 28 ++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/desc/grid.py b/desc/grid.py
index 9546dd733..c4b8a46e7 100644
--- a/desc/grid.py
+++ b/desc/grid.py
@@ -1648,25 +1648,27 @@ def most_rational(a, b, itol=1e-14):
     """
     a = float(_round(a, itol))
     b = float(_round(b, itol))
-    # handle empty range
+
+    # Handle empty range
     if a == b:
         return a
-    # ensure a < b
-    elif a > b:
-        c = a
-        a = b
-        b = c
-    # return 0 if in range
+
+    # Return 0 if in range
     if np.sign(a * b) <= 0:
         return 0
-    # handle negative ranges
-    elif np.sign(a) < 0:
+
+    # Handle negative ranges
+    if np.sign(a) < 0:
         s = -1
         a *= -1
         b *= -1
     else:
         s = 1
 
+    # Ensure a < b
+    if a > b:
+        a, b = b, a
+
     a_cf = dec_to_cf(a)
     b_cf = dec_to_cf(b)
     idx = 0  # first index of dissimilar digits
diff --git a/tests/test_grid.py b/tests/test_grid.py
index 929a1bbe5..b70e36d67 100644
--- a/tests/test_grid.py
+++ b/tests/test_grid.py
@@ -14,6 +14,7 @@
     dec_to_cf,
     find_least_rational_surfaces,
     find_most_rational_surfaces,
+    most_rational,
 )
 from desc.profiles import PowerSeriesProfile
 
@@ -823,6 +824,33 @@ def test_find_most_rational_surfaces():
     np.testing.assert_allclose(rho, np.linspace(0, 1, 5), atol=1e-14, rtol=0)
     np.testing.assert_allclose(io, np.linspace(1, 3, 5), atol=1e-14, rtol=0)
 
+    # simple test, linear iota going from -1 to -3
+    iota = PowerSeriesProfile([-1, -2])
+    rho, io = find_most_rational_surfaces(iota, 5)
+    np.testing.assert_allclose(rho, np.linspace(0, 1, 5), atol=1e-14, rtol=0)
+    np.testing.assert_allclose(io, np.linspace(-1, -3, 5), atol=1e-14, rtol=0)
+
+    # invalid (a > b) ranges and negative ranges are swapped and made positive
+    all_same = [
+        most_rational(1, 2),
+        most_rational(2, 1),
+        most_rational(-1, -2),
+        most_rational(-2, -1),
+    ]
+
+    assert len(set(map(abs, all_same))) == 1
+
+    # if 0 in range, return 0
+    has_zero = [
+        most_rational(0, 1),
+        most_rational(0, -1),
+        most_rational(0, 0),
+        most_rational(-1, 0),
+        most_rational(1, 0),
+    ]
+
+    assert all(result == 0 for result in has_zero)
+
 
 @pytest.mark.unit
 def test_find_least_rational_surfaces():