Repository: sunpy/sunpy

Issue:
Add a way of specifying rotation number and longitude to carrington_rotation_time
Currently `carrington_rotation_time` accepts a floating point number as input, which is the fractional Carrington rotation number. However, calculating this number from a rotation number and Carrington longitude is a bit confusing, because Carrington longitudes run backwards in time, going from 360 > 0. e.g.

```
2210, 90 deg > 2210.75
2210, 180 deg > 2210.5
2210, 270 deg > 2210.25
```
(note that in the order written, these numbers/coordinates are going backwards in time)

It would be nice if the API somehow allowed one to input rotation number and Longitude, so it was easy to input these values directly read from a synoptic map (e.g. https://sdo.gsfc.nasa.gov/data/synoptic/).

Discussion:
pep8speaks: Hello @kc611! Thanks for opening this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:

* In the file [`sunpy/coordinates/sun.py`](https://github.com/sunpy/sunpy/blob/79cd28291b683e91116a3a907692191821a9b598/sunpy/coordinates/sun.py):

> [Line 104:101](https://github.com/sunpy/sunpy/blob/79cd28291b683e91116a3a907692191821a9b598/sunpy/coordinates/sun.py#L104): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (102 > 100 characters)
> [Line 105:101](https://github.com/sunpy/sunpy/blob/79cd28291b683e91116a3a907692191821a9b598/sunpy/coordinates/sun.py#L105): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (104 > 100 characters)
> [Line 105:105](https://github.com/sunpy/sunpy/blob/79cd28291b683e91116a3a907692191821a9b598/sunpy/coordinates/sun.py#L105): [W291](https://duckduckgo.com/?q=pep8%20W291) trailing whitespace
> [Line 109:101](https://github.com/sunpy/sunpy/blob/79cd28291b683e91116a3a907692191821a9b598/sunpy/coordinates/sun.py#L109): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (102 > 100 characters)
> [Line 109:103](https://github.com/sunpy/sunpy/blob/79cd28291b683e91116a3a907692191821a9b598/sunpy/coordinates/sun.py#L109): [W291](https://duckduckgo.com/?q=pep8%20W291) trailing whitespace
> [Line 110:101](https://github.com/sunpy/sunpy/blob/79cd28291b683e91116a3a907692191821a9b598/sunpy/coordinates/sun.py#L110): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (103 > 100 characters)
> [Line 110:104](https://github.com/sunpy/sunpy/blob/79cd28291b683e91116a3a907692191821a9b598/sunpy/coordinates/sun.py#L110): [W291](https://duckduckgo.com/?q=pep8%20W291) trailing whitespace
> [Line 112:100](https://github.com/sunpy/sunpy/blob/79cd28291b683e91116a3a907692191821a9b598/sunpy/coordinates/sun.py#L112): [W291](https://duckduckgo.com/?q=pep8%20W291) trailing whitespace
> [Line 120:36](https://github.com/sunpy/sunpy/blob/79cd28291b683e91116a3a907692191821a9b598/sunpy/coordinates/sun.py#L120): [E251](https://duckduckgo.com/?q=pep8%20E251) unexpected spaces around keyword / parameter equals
> [Line 120:38](https://github.com/sunpy/sunpy/blob/79cd28291b683e91116a3a907692191821a9b598/sunpy/coordinates/sun.py#L120): [E251](https://duckduckgo.com/?q=pep8%20E251) unexpected spaces around keyword / parameter equals
> [Line 124:64](https://github.com/sunpy/sunpy/blob/79cd28291b683e91116a3a907692191821a9b598/sunpy/coordinates/sun.py#L124): [E251](https://duckduckgo.com/?q=pep8%20E251) unexpected spaces around keyword / parameter equals
> [Line 124:66](https://github.com/sunpy/sunpy/blob/79cd28291b683e91116a3a907692191821a9b598/sunpy/coordinates/sun.py#L124): [E251](https://duckduckgo.com/?q=pep8%20E251) unexpected spaces around keyword / parameter equals
> [Line 126:59](https://github.com/sunpy/sunpy/blob/79cd28291b683e91116a3a907692191821a9b598/sunpy/coordinates/sun.py#L126): [E251](https://duckduckgo.com/?q=pep8%20E251) unexpected spaces around keyword / parameter equals
> [Line 126:61](https://github.com/sunpy/sunpy/blob/79cd28291b683e91116a3a907692191821a9b598/sunpy/coordinates/sun.py#L126): [E251](https://duckduckgo.com/?q=pep8%20E251) unexpected spaces around keyword / parameter equals


kc611: > A test needs to be added to check that the function is returning the right values when longitude is given.

Can these values be simply hard-coded ? Also should tests be also added for checking whether it throws errors correctly ?

( I'll add these once the main function has been finalized. )
dstansby: Code is looking good! One more thing I noticed is that there should be an error if `longitude` is not in the range 0 to 360 degrees.

For the tests, I think it would be good to add:
- A check that passing a floating point rotation number and `longitude` raises an error
- A check that passing `longitude=360 * u.deg` returns the same result as not passing longitude
- A check that passing `crot=1, longitude=360*u.deg` gives the same result as `crot=2`
- A check that passing `longitude=400*u.deg` or `longitude=-10*u.deg` raises an error
ayshih: 
> - A check that passing `longitude=360 * u.deg` returns the same result as not passing longitude
> - A check that passing `crot=1, longitude=360*u.deg` gives the same result as `crot=2`

Presumably you mean `0*u.deg` for the second check.  The fact that `0*u.deg` and `360*u.deg` are expected to give different answers is a bit disconcerting; for example, one wouldn't be able to use the `Longitude` class.

Other comments:
- The docstring needs to explain this mode of providing the `longtiude` argument before actually getting to the parameter list.
- The type check for the rotation number is a bit odd.  We're not going to accept a float with zero fractional part?
- The broadcasting code looks clunky and not sufficiently general, and I'm not sure why it's even needed.  Why isn't the broadcasting already taken care of automatically in the arithmetic expression with `crot` and `longitude`?
dstansby: > Presumably you mean `0*u.deg` for the second check. The fact that `0*u.deg` and `360*u.deg` are expected to give different answers is a bit disconcerting; for example, one wouldn't be able to use the `Longitude` class.

Yes, thanks for catching, I've edited it now. They're different in the sense that 00:00 and 24:00 are different times (the beginning and end of a day), but they are both "midnight". To reduce confusion, I'd suggest that we limit the longitude values to (0, 360], ie. make the interval open on the left and disallow 0 degrees input.
kc611: @dstansby @ayshih Sorry for late response. I think the latest commit addresses all the points highlighted by you.

Patch:
diff --git a/changelog/4879.feature.rst b/changelog/4879.feature.rst
new file mode 100644
index 000000000..0a15f09fc
--- /dev/null
+++ b/changelog/4879.feature.rst
@@ -0,0 +1,1 @@
+Added a ``longitude`` keyword argument to :func:`~sunpy.coordinates.sun.carrington_rotation_time` as an alternate way to specify a fractional Carrington rotation.
diff --git a/sunpy/coordinates/sun.py b/sunpy/coordinates/sun.py
index 43bbbf687..a528f6114 100644
--- a/sunpy/coordinates/sun.py
+++ b/sunpy/coordinates/sun.py
@@ -22,6 +22,7 @@ from astropy.coordinates.builtin_frames.utils import get_jd12
 from astropy.coordinates.representation import CartesianRepresentation, SphericalRepresentation
 # Import erfa via astropy to make sure we are using the same ERFA library as Astropy
 from astropy.coordinates.sky_coordinate import erfa
+from astropy.tests.helper import quantity_allclose
 from astropy.time import Time
 
 from sunpy import log
@@ -91,23 +92,52 @@ def sky_position(t='now', equinox_of_date=True):
     return ra, dec
 
 
-def carrington_rotation_time(crot):
+@u.quantity_input
+def carrington_rotation_time(crot, longitude: u.deg = None):
     """
     Return the time of a given Carrington rotation.
 
+    Fractional Carrington rotation numbers can be provided in two ways:
+    * Fractional numbers to ``crot``
+    * Integer numbers to ``crot`` and a Carrington longitude to ``longitude``
+
+    Inputs can be arrays.  If both ``crot`` and ``longitude`` are provided, the
+    output shape will be the broadcasted combination.
     The round-trip from this method to `carrington_rotation_number` has
     absolute errors of < 0.11 seconds.
 
     Parameters
     ----------
-    crot : float
-        Carrington rotation number. Can be a fractional cycle number.
+    crot : `int`, `float`, `~astropy.units.Quantity`
+        Carrington rotation number(s). Can be a fractional rotation number.
+
+    longitude : `~astropy.units.Quantity`
+        Carrington longitude(s), which must be > 0 degrees and <= 360 degrees.
+        If provided, ``crot`` must be strictly integral.
 
     Returns
     -------
-    astropy.time.Time
-    """
-    estimate = (constants.mean_synodic_period.to_value('day') *
+    `astropy.time.Time`
+
+    Examples
+    --------
+    >>> from sunpy.coordinates.sun import carrington_rotation_time
+    >>> import astropy.units as u
+    >>> carrington_rotation_time(2242)
+    <Time object: scale='utc' format='iso' value=2021-03-17 22:31:37.030>
+    >>> carrington_rotation_time(2000.25)
+    <Time object: scale='utc' format='iso' value=2003-02-27 02:52:57.315>
+    >>> carrington_rotation_time(2000, 270*u.deg)
+    <Time object: scale='utc' format='iso' value=2003-02-27 02:52:57.315>
+    """
+    crot = crot << u.one
+    if longitude is not None:
+        if not quantity_allclose(crot%1, 0):
+            raise ValueError("Carrington rotation number(s) must be integral if `longitude` is provided.")
+        if (longitude <= 0*u.deg).any() or (longitude > 360*u.deg).any():
+            raise ValueError("Carrington longitude(s) must be > 0 degrees and <= 360 degrees.")
+        crot = crot + (1 - longitude/(360*u.deg))
+    estimate = (constants.mean_synodic_period *
                 (crot - 1)) + constants.first_carrington_rotation
 
     # The above estimate is inaccurate (see comments below in carrington_rotation_number),


Test patch:
diff --git a/sunpy/coordinates/tests/test_sun.py b/sunpy/coordinates/tests/test_sun.py
index 50b02ae3b..087bae803 100644
--- a/sunpy/coordinates/tests/test_sun.py
+++ b/sunpy/coordinates/tests/test_sun.py
@@ -1,5 +1,6 @@
 import warnings
 
+import numpy as np
 import pytest
 from numpy.testing import assert_allclose
 
@@ -472,6 +473,39 @@ def test_carrington_rotation_starttime(crot, julian_days):
                                  julian_days * u.day, atol=0.11*u.s)
 
 
+@pytest.mark.parametrize("crot, longitude, crot_fractional",
+                         [(2000, 360, 2000.0),
+                          (2000.0, 270, 2000.25)
+                          ])
+def test_carrington_rotation_time_longitude(crot, longitude, crot_fractional):
+    assert sun.carrington_rotation_time(crot*u.one, longitude*u.deg) == \
+        sun.carrington_rotation_time(crot_fractional*u.one)
+
+
+@pytest.mark.parametrize("crot, longitude, crot_fractional",
+                         [
+                             (np.array([2000, 2000]), np.array(
+                                 [180, 90]), np.array([2000.5, 2000.75])),
+                             (2000, np.array([180, 90]), np.array([2000.5, 2000.75])),
+                             (np.array([2000, 2000]), 180, np.array([2000.5, 2000.5]))
+                         ])
+def test_carrington_rotation_time_longitude_numpy(crot, longitude, crot_fractional):
+    assert all(sun.carrington_rotation_time(crot*u.one, longitude*u.deg) ==
+               sun.carrington_rotation_time(crot_fractional*u.one))
+
+
+@pytest.mark.parametrize("crot, longitude",
+                         [
+                             (2000, 0),
+                             (2000, -10),
+                             (2000, 400),
+                             (2000.5, 180),
+                         ])
+def test_carrington_rotation_time_longitude_err(crot, longitude):
+    with pytest.raises(ValueError):
+        sun.carrington_rotation_time(crot*u.one, longitude*u.deg)
+
+
 def test_carrington_rotation_roundtrip():
     t = Time('2010-1-1')
     crot = sun.carrington_rotation_number(t)