Apply suggestions from code review

Co-authored-by: Eero Vaher <eero.vaher@gmail.com>
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>

astropy/cosmology/io/ecsv.py

@@ -34,7 +34,7 @@ def read_ecsv(
         will be ``{'key': 10}``).
 
     rename : dict or None (optional keyword-only)
-        A dictionary mapping column names to fields of the Cosmology.
+        A dictionary mapping column names to fields of the `~astropy.cosmology.Cosmology`.
 
     **kwargs
         Passed to :attr:`astropy.table.QTable.read`
@@ -71,7 +71,7 @@ def write_ecsv(
 
     Parameters
     ----------
-    cosmology : |Cosmology|
+    cosmology : `~astropy.cosmology.Cosmology`
         The cosmology instance to convert to a mapping.
     file : path-like or file-like
         Location to save the serialized cosmology.
@@ -85,7 +85,7 @@ def write_ecsv(
         Whether to put the cosmology class in the Table metadata (if `True`,
         default) or as the first column (if `False`).
     rename : dict or None (optional keyword-only)
-        A dictionary mapping fields of the Cosmology to columns of the table.
+        A dictionary mapping fields of the `~astropy.cosmology.Cosmology` to columns of the table.
 
     **kwargs
         Passed to ``cls.write``

astropy/cosmology/io/mapping.py

@@ -20,8 +20,11 @@
 def _rename_map(map, /, renames):
     # So we are guaranteed to have a poppable map.
     # And apply renames, if given.
-    if not set(renames.values()).isdisjoint(map.keys()):
-        raise ValueError("'renames' values must be disjoint from 'map' keys.")
+    if common_names := set(renames.values()).intersection(map):
+        raise ValueError(
+            "'renames' values must be disjoint from 'map' keys, "
+            f"the common keys are: {common_names}"
+        )
     return {renames.get(k, k): v for k, v in map.items()}
 
 
@@ -34,9 +37,7 @@ def _get_cosmology_class(cosmology, params, /):
     else:
         params.pop("cosmology", None)  # pop, but don't use
     # if string, parse to class
-    if isinstance(cosmology, str):
-        cosmology = _COSMOLOGY_CLASSES[cosmology]
-    return cosmology
+    return _COSMOLOGY_CLASSES[cosmology] if isinstance(cosmology, str) else cosmology
 
 
 def from_mapping(map, *, move_to_meta=False, cosmology=None, rename=None):
@@ -65,7 +66,8 @@ class itself.
         filling in any non-mandatory arguments missing in 'map'.
 
     rename : dict or None (optional, keyword-only)
-        A dictionary mapping keys in 'map' to fields of the Cosmology.
+        A dictionary mapping keys in ``map`` to fields of the
+        `~astropy.cosmology.Cosmology`.
 
     Returns
     -------
@@ -106,7 +108,7 @@ class itself.
     """
     # Rename keys, if given a ``renames`` dict.
     # Also, make a copy of the mapping, so we can pop from it.
-    params = _rename_map(dict(map), renames={} if rename is None else rename)
+    params = _rename_map(dict(map), renames=rename or {})
 
     # Get cosmology class
     cosmology = _get_cosmology_class(cosmology, params)
@@ -145,7 +147,7 @@ def to_mapping(
 
     Parameters
     ----------
-    cosmology : |Cosmology|
+    cosmology : :class:`~astropy.cosmology.Cosmology`
         The cosmology instance to convert to a mapping.
     *args
         Not used. Needed for compatibility with
@@ -161,7 +163,8 @@ def to_mapping(
         `False`, default) or to merge with the rest of the mapping, preferring
         the original values (if `True`)
     rename : dict or None (optional, keyword-only)
-        A dictionary mapping fields of the Cosmology to keys in the map.
+        A `dict` mapping fields of the :class:`~astropy.cosmology.Cosmology` to keys in
+        the map.
 
     Returns
     -------
@@ -252,9 +255,7 @@ def to_mapping(
     if not move_from_meta:
         m["meta"] = meta  # TODO? should meta be type(cls)
     # Rename keys
-    if rename is not None:
-        m = _rename_map(m, rename)
-    return m
+    return m if rename is None else _rename_map(m, rename)
 
 
 def mapping_identify(origin, format, *args, **kwargs):

astropy/cosmology/io/row.py

@@ -11,7 +11,7 @@
 
 
 def from_row(row, *, move_to_meta=False, cosmology=None, rename=None):
-    """Instantiate a |Cosmology| from a `~astropy.table.Row`.
+    """Instantiate a `~astropy.cosmology.Cosmology` from a `~astropy.table.Row`.
 
     Parameters
     ----------
@@ -32,11 +32,11 @@ def from_row(row, *, move_to_meta=False, cosmology=None, rename=None):
         filling in any non-mandatory arguments missing in 'table'.
 
     rename : dict or None (optional, keyword-only)
-        A dictionary mapping columns in the row to fields of the Cosmology.
+        A dictionary mapping columns in the row to fields of the `~astropy.cosmology.Cosmology`.
 
     Returns
     -------
-    |Cosmology| subclass instance
+    `~astropy.cosmology.Cosmology`
 
     Examples
     --------
@@ -63,12 +63,12 @@ def from_row(row, *, move_to_meta=False, cosmology=None, rename=None):
                       Tcmb0=2.7255 K, Neff=3.046, m_nu=[0. 0. 0.06] eV, Ob0=0.04897)
     """
     inv_rename = {v: k for k, v in rename.items()} if rename is not None else {}
-    kn = inv_rename.get("name", "name")
-    km = inv_rename.get("meta", "meta")
-    kc = inv_rename.get("cosmology", "cosmology")
+    kname = inv_rename.get("name", "name")
+    kmeta = inv_rename.get("meta", "meta")
+    kcosmo = inv_rename.get("cosmology", "cosmology")
 
     # special values
-    name = row[kn] if kn in row.columns else None  # get name from column
+    name = row.get(kname)
 
     meta = defaultdict(dict, copy.deepcopy(row.meta))
     # Now need to add the Columnar metadata. This is only available on the
@@ -80,9 +80,9 @@ def from_row(row, *, move_to_meta=False, cosmology=None, rename=None):
 
     # turn row into mapping, filling cosmo if not in a column
     mapping = dict(row)
-    mapping[kn] = name
-    mapping.setdefault(kc, meta.pop(kc, None))
-    mapping[km] = dict(meta)
+    mapping[kname] = name
+    mapping.setdefault(kcosmo, meta.pop(kcosmo, None))
+    mapping[kmeta] = dict(meta)
 
     # build cosmology from map
     return from_mapping(
@@ -95,7 +95,7 @@ def to_row(cosmology, *args, cosmology_in_meta=False, table_cls=QTable, rename=N
 
     Parameters
     ----------
-    cosmology : |Cosmology|
+    cosmology : `~astropy.cosmology.Cosmology`
         The cosmology instance to convert to a mapping.
     *args
         Not used. Needed for compatibility with
@@ -116,7 +116,7 @@ def to_row(cosmology, *args, cosmology_in_meta=False, table_cls=QTable, rename=N
 
     Examples
     --------
-    A Cosmology as a `~astropy.table.Row` will have the cosmology's name and parameters
+    A `~astropy.cosmology.Cosmology` as a `~astropy.table.Row` will have the cosmology's name and parameters
     as columns.
 
         >>> from astropy.cosmology import Planck18

astropy/cosmology/io/table.py

@@ -33,17 +33,17 @@ def from_table(table, index=None, *, move_to_meta=False, cosmology=None, rename=
         (e.g. for ``Cosmology(meta={'key':10}, key=42)``, the ``Cosmology.meta``
         will be ``{'key': 10}``).
 
-    cosmology : str or |Cosmology| class or None (optional, keyword-only)
+    cosmology : str or type or None (optional, keyword-only)
         The cosmology class (or string name thereof) to use when constructing
         the cosmology instance. The class also provides default parameter values,
         filling in any non-mandatory arguments missing in 'table'.
 
     rename : dict or None (optional, keyword-only)
-        A dictionary mapping columns in 'table' to fields of the Cosmology class.
+        A dictionary mapping columns in 'table' to fields of the `~astropy.cosmology.Cosmology` class.
 
     Returns
     -------
-    |Cosmology| subclass instance
+    `~astropy.cosmology.Cosmology`
 
     Examples
     --------
@@ -115,7 +115,10 @@ def from_table(table, index=None, *, move_to_meta=False, cosmology=None, rename=
         if not table.indices:  # no indexing column, find by string match
             nc = "name"  # default name column
             if rename is not None:  # from inverted `rename`
-                nc = {v: k for k, v in rename.items()}.get(nc, nc)
+                for key, value in rename.items():
+                    if value == "name":
+                        nc = key
+                        break
 
             indices = np.where(table[nc] == index)[0]
         else:  # has indexing column
@@ -150,7 +153,7 @@ def to_table(cosmology, *args, cls=QTable, cosmology_in_meta=True, rename=None):
 
     Parameters
     ----------
-    cosmology : |Cosmology| subclass instance
+    cosmology : `~astropy.cosmology.Cosmology`
         The cosmology instance to convert to a table.
     *args
         Not used. Needed for compatibility with
@@ -242,7 +245,7 @@ def to_table(cosmology, *args, cls=QTable, cosmology_in_meta=True, rename=None):
     tbl = cls(data, meta=meta)
 
     # Renames
-    renames = {} if rename is None else rename
+    renames = rename or {}
     for name in tbl.colnames:
         tbl.rename_column(name, renames.get(name, name))
 

astropy/cosmology/io/tests/test_ecsv.py

@@ -127,7 +127,7 @@ def test_readwrite_ecsv_instance(
         got = read(fp)
         assert got == cosmo
 
-    def test_readwrite_ecsv_rename(
+    def test_readwrite_ecsv_renamed_columns(
         self, cosmo_cls, cosmo, read, write, tmp_path, add_cu
     ):
         """Test rename argument to read/write."""

astropy/cosmology/io/tests/test_mapping.py

@@ -129,29 +129,26 @@ def test_tofrom_mapping_move_tofrom_meta(self, cosmo, to_format, from_format):
         assert got == cosmo  # (Doesn't check metadata)
         assert got.meta["mismatching"] == "will error"
 
-    def test_to_mapping_rename_disjoint(self, cosmo, to_format):
-        """Test roundtrip of ``rename`` in ``to/from_mapping()``."""
-        # rename
+    def test_to_mapping_rename_conflict(self, cosmo, to_format):
+        """Test ``rename`` in ``to_mapping()``."""
         to_rename = {"name": "name"}
-        with pytest.raises(ValueError, match="disjoint"):
+        with pytest.raises(ValueError, match="'renames' values must be disjoint"):
             to_format("mapping", rename=to_rename)
 
-    def test_from_mapping_rename_disjoint(self, cosmo, to_format, from_format):
-        """Test roundtrip of ``rename`` in ``to/from_mapping()``."""
-        # rename
+    def test_from_mapping_rename_conflict(self, cosmo, to_format, from_format):
+        """Test ``rename`` in `from_mapping()``."""
         m = to_format("mapping")
 
-        with pytest.raises(ValueError, match="disjoint"):
+        with pytest.raises(ValueError, match="'renames' values must be disjoint"):
             from_format(m, format="mapping", rename={"name": "name"})
 
-    def test_tofrom_mapping_rename(self, cosmo, to_format, from_format):
-        """Test roundtrip of ``rename`` in ``to/from_mapping()``."""
-        # rename
+    def test_tofrom_mapping_rename_roundtrip(self, cosmo, to_format, from_format):
+        """Test roundtrip in ``to/from_mapping()`` with ``rename``."""
         to_rename = {"name": "cosmo_name"}
         m = to_format("mapping", rename=to_rename)
 
-        assert "name" not in m.keys()
-        assert "cosmo_name" in m.keys()
+        assert "name" not in m
+        assert "cosmo_name" in m
 
         # Wrong names = error
         with pytest.raises(TypeError, match="there are unused parameters"):