Repository: astropy/astropy

Issue:
Possible speed-ups for `get_grouped_by_powers`
This function uses `.append`. It looks like with a simple helper function (defined externally to this function) we could use a list comprehension instead. I wonder if that would be faster — `append` versus function call overhead.

_Originally posted by @nstarman in https://github.com/astropy/astropy/pull/16392#discussion_r1591049850_

`itertools` has a recipe for this type of dual-list creation loop. Doing two loops at the C-level might be faster than 1 loop with constant appending.
            

Discussion:
github-actions[bot]: Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  - [ ] Do the proposed changes actually accomplish desired goals?
  - [ ] Do the proposed changes follow the [Astropy coding guidelines](https://docs.astropy.org/en/latest/development/codeguide.html)?
  - [ ] Are tests added/updated as required? If so, do they follow the [Astropy testing guidelines](https://docs.astropy.org/en/latest/development/testguide.html)?
  - [ ] Are docs added/updated as required? If so, do they follow the [Astropy documentation guidelines](https://docs.astropy.org/en/latest/development/docguide.html#astropy-documentation-rules-and-guidelines)?
  - [ ] Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for [rebase](https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#rebase-if-necessary) and [squash](https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#squash-if-necessary).
  - [ ] Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the [bot](https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#pre-commit).
  - [ ] Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  - [ ] Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  - [ ] At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) *before* merge.
eerovaher: When I last rebased this pull request I also edited the tests that are meant to check the errors that are raised if an incorrect `fraction` value is passed to the `to_string()` methods. The updates make it clear that even the formatters that support `fraction="multiline"` nonetheless say that they only support `fraction="inline"` in the error message, and that the updated code corrects that mistake. But this mistake is not simple to fix if `Console` inherits its `to_string()` from `Base` only changing the default `fraction` value, which means that the code duplication between the proposed `Base.to_string()` and `Console.to_string()` is less problematic than it first might have appeared to have been.
eerovaher: I do have an idea now how to avoid code duplication in the `Base.to_string()` and `Console.to_string()` methods, and if it works we wouldn't need `Base.to_string_fraction_helper()` as a separate method because the code could reside in `Base.to_string()`. I've converted this pull request to a draft while I'm working on that idea.
eerovaher: My new approach to minimize code duplication is to introduce the `_fraction_formatters` class property, which maps accepted `to_string()` `fraction` argument values to the corresponding formatter functions. The only downside is that the current implementation is just slightly too dynamic for static type checkers, but we are not type checking yet.

Patch:
diff --git a/astropy/units/format/base.py b/astropy/units/format/base.py
index 1c417d92cd..5644f11581 100644
--- a/astropy/units/format/base.py
+++ b/astropy/units/format/base.py
@@ -4,10 +4,12 @@ from __future__ import annotations
 
 from typing import TYPE_CHECKING
 
+from astropy.utils import classproperty
+
 from . import utils
 
 if TYPE_CHECKING:
-    from collections.abc import Iterable
+    from collections.abc import Callable, Iterable
     from typing import ClassVar, Literal
 
     import numpy as np
@@ -42,6 +44,13 @@ class Base:
         Base.registry[cls.name] = cls
         super().__init_subclass__(**kwargs)
 
+    @classproperty(lazy=True)
+    def _fraction_formatters(cls) -> dict[bool | str, Callable[[str, str, str], str]]:
+        return {
+            True: cls._format_inline_fraction,
+            "inline": cls._format_inline_fraction,
+        }
+
     @classmethod
     def format_exponential_notation(
         cls, val: float | np.number, format_spec: str = ".8g"
@@ -101,20 +110,9 @@ class Base:
         )
 
     @classmethod
-    def _format_fraction(
-        cls,
-        scale: str,
-        numerator: str,
-        denominator: str,
-        *,
-        fraction: Literal[True, "inline"] = "inline",
+    def _format_inline_fraction(
+        cls, scale: str, numerator: str, denominator: str
     ) -> str:
-        if not (fraction is True or fraction == "inline"):
-            raise ValueError(
-                "format {cls.name!r} only supports inline fractions,"
-                f"not fraction={fraction!r}."
-            )
-
         if cls._space in denominator:
             denominator = f"({denominator})"
         if scale and numerator == "1":
@@ -123,7 +121,7 @@ class Base:
 
     @classmethod
     def to_string(
-        cls, unit: UnitBase, *, fraction: bool | Literal["inline", "multiline"] = True
+        cls, unit: UnitBase, *, fraction: bool | Literal["inline"] = True
     ) -> str:
         """Convert a unit to its string representation.
 
@@ -151,34 +149,44 @@ class Base:
         # First the scale.  Normally unity, in which case we omit
         # it, but non-unity scale can happen, e.g., in decompositions
         # like u.Ry.decompose(), which gives "2.17987e-18 kg m2 / s2".
-        if unit.scale == 1:
-            s = ""
-        else:
-            s = cls.format_exponential_notation(unit.scale)
-
-        # Now the unit baes, taking care that dimensionless does not have any
-        # (but can have a scale; e.g., u.percent.decompose() gives "0.01").
-        if len(unit.bases):
-            if s:
-                s += cls._scale_unit_separator
-            if fraction:
-                numerator, denominator = utils.get_grouped_by_powers(
-                    unit.bases, unit.powers
-                )
-            else:
-                numerator = list(zip(unit.bases, unit.powers))
-                denominator = []
-            if len(denominator):
-                if len(numerator):
-                    numerator = cls._format_unit_list(numerator)
-                else:
-                    numerator = "1"
-                denominator = cls._format_unit_list(denominator)
-                s = cls._format_fraction(s, numerator, denominator, fraction=fraction)
+        s = "" if unit.scale == 1 else cls.format_exponential_notation(unit.scale)
+
+        # dimensionless does not have any bases, but can have a scale;
+        # e.g., u.percent.decompose() gives "0.01".
+        if not unit.bases:
+            return s
+
+        if s:
+            s += cls._scale_unit_separator
+        # Unit powers are monotonically decreasing
+        if not fraction or unit.powers[-1] > 0:
+            return s + cls._format_unit_list(zip(unit.bases, unit.powers, strict=True))
+
+        positive = []
+        negative = []
+        for base, power in zip(unit.bases, unit.powers, strict=True):
+            if power > 0:
+                positive.append((base, power))
             else:
-                s += cls._format_unit_list(numerator)
-
-        return s
+                negative.append((base, -power))
+        try:
+            return cls._fraction_formatters[fraction](
+                s,
+                cls._format_unit_list(positive) or "1",
+                cls._format_unit_list(negative),
+            )
+        except KeyError:
+            # We accept Booleans, but don't advertise them in the error message
+            *all_but_last, last = (
+                repr(key) for key in cls._fraction_formatters if isinstance(key, str)
+            )
+            supported_formats = (
+                f"{', '.join(all_but_last)} or {last}" if all_but_last else last
+            )
+            raise ValueError(
+                f"{cls.name!r} format only supports {supported_formats} "
+                f"fractions, not {fraction=!r}."
+            ) from None
 
     @classmethod
     def parse(cls, s: str) -> UnitBase:
diff --git a/astropy/units/format/cds.py b/astropy/units/format/cds.py
index 8ba62f3d8f..e92327fb2b 100644
--- a/astropy/units/format/cds.py
+++ b/astropy/units/format/cds.py
@@ -299,7 +299,7 @@ class CDS(Generic):
 
     @classmethod
     def to_string(
-        cls, unit: UnitBase, fraction: bool | Literal["inline", "multiline"] = False
+        cls, unit: UnitBase, fraction: bool | Literal["inline"] = False
     ) -> str:
         # Remove units that aren't known to the format
         unit = utils.decompose_to_known_units(
diff --git a/astropy/units/format/console.py b/astropy/units/format/console.py
index b350d22dac..c3586a06e2 100644
--- a/astropy/units/format/console.py
+++ b/astropy/units/format/console.py
@@ -8,9 +8,12 @@ from __future__ import annotations
 
 from typing import TYPE_CHECKING
 
+from astropy.utils import classproperty
+
 from . import base
 
 if TYPE_CHECKING:
+    from collections.abc import Callable
     from typing import ClassVar, Literal
 
     from astropy.units import UnitBase
@@ -37,24 +40,20 @@ class Console(base.Base):
     _line: ClassVar[str] = "-"
     _space: ClassVar[str] = " "
 
+    @classproperty(lazy=True)
+    def _fraction_formatters(cls) -> dict[bool | str, Callable[[str, str, str], str]]:
+        return super()._fraction_formatters | {
+            "multiline": cls._format_multiline_fraction
+        }
+
     @classmethod
     def _format_superscript(cls, number: str) -> str:
         return f"^{number}"
 
     @classmethod
-    def _format_fraction(
-        cls,
-        scale: str,
-        numerator: str,
-        denominator: str,
-        *,
-        fraction: Literal[True, "inline", "multiline"] = "multiline",
+    def _format_multiline_fraction(
+        cls, scale: str, numerator: str, denominator: str
     ) -> str:
-        if fraction != "multiline":
-            return super()._format_fraction(
-                scale, numerator, denominator, fraction=fraction
-            )
-
         fraclength = max(len(numerator), len(denominator))
         f = f"{{0:<{len(scale)}s}}{{1:^{fraclength}s}}"
 
diff --git a/astropy/units/format/latex.py b/astropy/units/format/latex.py
index 500b39e9a1..13af2b8a8d 100644
--- a/astropy/units/format/latex.py
+++ b/astropy/units/format/latex.py
@@ -57,19 +57,9 @@ class Latex(console.Console):
         return name
 
     @classmethod
-    def _format_fraction(
-        cls,
-        scale: str,
-        numerator: str,
-        denominator: str,
-        *,
-        fraction: Literal[True, "inline", "multiline"] = "multiline",
+    def _format_multiline_fraction(
+        cls, scale: str, numerator: str, denominator: str
     ) -> str:
-        if fraction != "multiline":
-            return super()._format_fraction(
-                scale, numerator, denominator, fraction=fraction
-            )
-
         return rf"{scale}\frac{{{numerator}}}{{{denominator}}}"
 
     @classmethod
diff --git a/astropy/units/format/utils.py b/astropy/units/format/utils.py
index 156b4982b7..ecb63298cb 100644
--- a/astropy/units/format/utils.py
+++ b/astropy/units/format/utils.py
@@ -13,46 +13,10 @@ from astropy.units.utils import maybe_simple_fraction
 
 if TYPE_CHECKING:
     from collections.abc import Callable, Generator, Iterable, Sequence
-    from typing import TypeVar
 
     from astropy.units import UnitBase
     from astropy.units.typing import Real
 
-    T = TypeVar("T")
-
-
-def get_grouped_by_powers(
-    bases: Sequence[T], powers: Sequence[int]
-) -> tuple[list[tuple[T, int]], list[tuple[T, int]]]:
-    """
-    Groups the powers and bases in the given
-    `~astropy.units.CompositeUnit` into positive powers and
-    negative powers for easy display on either side of a solidus.
-
-    Parameters
-    ----------
-    bases : list of `astropy.units.UnitBase` instances
-
-    powers : list of int
-
-    Returns
-    -------
-    positives, negatives : list of tuple of |Unit| and power
-       Each element in each list is tuple of the form (*base*,
-       *power*).  The negatives have the sign of their power reversed
-       (i.e. the powers are all positive).
-    """
-    positive = []
-    negative = []
-    for base, power in zip(bases, powers):
-        if power < 0:
-            negative.append((base, -power))
-        elif power > 0:
-            positive.append((base, power))
-        else:
-            raise ValueError("Unit with 0 power")
-    return positive, negative
-
 
 def decompose_to_known_units(
     unit: UnitBase, func: Callable[[UnitBase], None]
diff --git a/astropy/units/format/vounit.py b/astropy/units/format/vounit.py
index f9ecc6a1ab..02cc22ed65 100644
--- a/astropy/units/format/vounit.py
+++ b/astropy/units/format/vounit.py
@@ -171,13 +171,9 @@ class VOUnit(generic.Generic):
         return super().format_exponential_notation(val, format_spec)
 
     @classmethod
-    def _format_fraction(cls, scale, numerator, denominator, *, fraction="inline"):
-        if not (fraction is True or fraction == "inline"):
-            raise ValueError(
-                "format {cls.name!r} only supports inline fractions,"
-                f"not fraction={fraction!r}."
-            )
-
+    def _format_inline_fraction(
+        cls, scale: str, numerator: str, denominator: str
+    ) -> str:
         if cls._space in denominator:
             denominator = f"({denominator})"
         if scale and numerator == "1":


Test patch:
diff --git a/astropy/units/tests/test_format.py b/astropy/units/tests/test_format.py
index fef2c74ade..9434cf6084 100644
--- a/astropy/units/tests/test_format.py
+++ b/astropy/units/tests/test_format.py
@@ -569,17 +569,26 @@ def test_format_styles_non_default_fraction(format_spec, fraction, string, decom
 @pytest.mark.parametrize("format_spec", ["generic", "cds", "fits", "ogip", "vounit"])
 def test_no_multiline_fraction(format_spec):
     fluxunit = u.W / u.m**2
-    with pytest.raises(ValueError, match="only supports.*not fraction='multiline'"):
+    with pytest.raises(
+        ValueError,
+        match=(
+            f"^'{format_spec}' format only supports 'inline' fractions, "
+            r"not fraction='multiline'\.$"
+        ),
+    ):
         fluxunit.to_string(format_spec, fraction="multiline")
 
 
-@pytest.mark.parametrize(
-    "format_spec",
-    ["generic", "cds", "fits", "ogip", "vounit", "latex", "console", "unicode"],
-)
+@pytest.mark.parametrize("format_spec", ["latex", "console", "unicode"])
 def test_unknown_fraction_style(format_spec):
     fluxunit = u.W / u.m**2
-    with pytest.raises(ValueError, match="only supports.*parrot"):
+    with pytest.raises(
+        ValueError,
+        match=(
+            f"^'{format_spec}' format only supports 'inline' or 'multiline' fractions, "
+            r"not fraction='parrot'\.$"
+        ),
+    ):
         fluxunit.to_string(format_spec, fraction="parrot")