Repository: yt-project/unyt (python)

Issue:
Unit conversions with time/length are wrong
* unyt version: 1.0.7
* Python version: 3.6.8
* Operating System: macOS 10.14.2

### Description

When I create quantities with units of `time/length` the unit conversions (e.g., `in_mks` or `in_cgs`) always seem to produce a nonsensical quantity with units of ohms. There is no error or warning produced, just incorrect results.

### What I Did

```
>>> import unyt

# This is to show that units of length/time works fine:
>>> a = unyt.unyt_quantity(100, 'm/s')
>>> a.in_cgs()
unyt_quantity(10000., 'cm/s')

# This is wrong:
>>> b = unyt.unyt_quantity(100, 's/m')
>>> b.in_cgs()
unyt_quantity(8.98755179e+13, 'ohm')
>>> b.in_mks()
unyt_quantity(8.98755179e+13, 'ohm')

# Also:
>>> a.in_cgs().in_cgs()
unyt_quantity(10000., 'cm/s')
>>> b.in_cgs().in_cgs()
unyt_quantity(100., 'statohm')
```

Pull request description:
fix incorrect conversions between inverse velocities and ohm. 
closes #60 

@jzuhone if you could look at this that would be awesome.

There is a behavior change (see the changed test) but I think it's worth making the behavior change to avoid the obviously incorrect behavior in #60. In principle we could also fix #60 and avoid the behavior change by just removing the conversion between ohm and statohm from the `em_conversions` dictionary however the exact same problem could also happen for the other CGS EM units, just less often because they have more unusual dimensions that won't come up in a calculation as often. Better to just remove the possibility of the bug.

Thankfully we're about to do a 2.0 release so behavior changes like this are OK if they're worth it (although should be avoided if we possibly can).

Patch:
diff --git a/unyt/unit_object.py b/unyt/unit_object.py
index 2746629..b07d2c5 100644
--- a/unyt/unit_object.py
+++ b/unyt/unit_object.py
@@ -66,6 +66,7 @@ from unyt._physical_ratios import speed_of_light_cm_per_s
 from unyt.unit_registry import (
     default_unit_registry,
     _lookup_unit_symbol,
+    _split_prefix,
     UnitRegistry,
     UnitParseError,
 )
@@ -858,28 +859,32 @@ def _cancel_mul(expr, registry):
 # canonical unit to convert to in that system, and floating point
 # conversion factor
 em_conversions = {
-    dims.charge_mks: (dims.charge_cgs, "esu", 0.1 * speed_of_light_cm_per_s),
-    dims.charge_cgs: (dims.charge_mks, "C", 10.0 / speed_of_light_cm_per_s),
-    dims.magnetic_field_mks: (dims.magnetic_field_cgs, "gauss", 1.0e4),
-    dims.magnetic_field_cgs: (dims.magnetic_field_mks, "T", 1.0e-4),
-    dims.current_mks: (dims.current_cgs, "statA", 0.1 * speed_of_light_cm_per_s),
-    dims.current_cgs: (dims.current_mks, "A", 10.0 / speed_of_light_cm_per_s),
-    dims.electric_potential_mks: (
+    ("C", dims.charge_mks): (dims.charge_cgs, "esu", 0.1 * speed_of_light_cm_per_s),
+    ("esu", dims.charge_cgs): (dims.charge_mks, "C", 10.0 / speed_of_light_cm_per_s),
+    ("T", dims.magnetic_field_mks): (dims.magnetic_field_cgs, "gauss", 1.0e4),
+    ("gauss", dims.magnetic_field_cgs): (dims.magnetic_field_mks, "T", 1.0e-4),
+    ("A", dims.current_mks): (dims.current_cgs, "statA", 0.1 * speed_of_light_cm_per_s),
+    ("statA", dims.current_cgs): (
+        dims.current_mks,
+        "A",
+        10.0 / speed_of_light_cm_per_s,
+    ),
+    ("V", dims.electric_potential_mks): (
         dims.electric_potential_cgs,
         "statV",
         1.0e-8 * speed_of_light_cm_per_s,
     ),
-    dims.electric_potential_cgs: (
+    ("statV", dims.electric_potential_cgs): (
         dims.electric_potential_mks,
         "V",
         1.0e8 / speed_of_light_cm_per_s,
     ),
-    dims.resistance_mks: (
+    ("ohm", dims.resistance_mks): (
         dims.resistance_cgs,
         "statohm",
         1.0e9 / (speed_of_light_cm_per_s ** 2),
     ),
-    dims.resistance_cgs: (
+    ("statohm", dims.resistance_cgs): (
         dims.resistance_mks,
         "ohm",
         1.0e-9 * speed_of_light_cm_per_s ** 2,
@@ -929,9 +934,13 @@ def _check_em_conversion(unit, to_unit=None, unit_system=None, registry=None):
     em_map = ()
     if unit == to_unit:
         return em_map
-    if unit.dimensions in em_conversions:
-        em_info = em_conversions[unit.dimensions]
-        em_unit = Unit(em_info[1], registry=registry)
+    if unit.is_atomic:
+        prefix, unit_wo_prefix = _split_prefix(str(unit), unit.registry.lut)
+    else:
+        prefix, unit_wo_prefix = "", str(unit)
+    if (unit_wo_prefix, unit.dimensions) in em_conversions:
+        em_info = em_conversions[unit_wo_prefix, unit.dimensions]
+        em_unit = Unit(prefix + em_info[1], registry=registry)
         if to_unit is None:
             cmks_in_unit = current_mks in unit.dimensions.atoms()
             cmks_in_unit_system = unit_system.units_map[current_mks]
@@ -961,8 +970,8 @@ def _check_em_conversion(unit, to_unit=None, unit_system=None, registry=None):
                 continue
         except MissingMKSCurrent:
             raise MKSCGSConversionError(unit)
-        if budims in em_conversions:
-            conv_unit = em_conversions[budims][1]
+        if (bu, budims) in em_conversions:
+            conv_unit = em_conversions[bu, budims][1]
             if to_unit is not None:
                 for to_unit_atom in to_unit.expr.atoms():
                     bou = str(to_unit_atom)
diff --git a/unyt/unit_registry.py b/unyt/unit_registry.py
index 2c61def..119be29 100644
--- a/unyt/unit_registry.py
+++ b/unyt/unit_registry.py
@@ -246,6 +246,28 @@ class UnitRegistry:
         return equiv
 
 
+def _split_prefix(symbol_str, unit_symbol_lut):
+    possible_prefix = symbol_str[0]
+
+    if symbol_str[:2] == "da":
+        possible_prefix = "da"
+
+    if possible_prefix in unit_prefixes:
+        # the first character could be a prefix, check the rest of the symbol
+        symbol_wo_pref = symbol_str[1:]
+
+        # deca is the only prefix with length 2
+        if symbol_str[:2] == "da":
+            symbol_wo_pref = symbol_str[2:]
+            possible_prefix = "da"
+
+        prefixable_units = [u for u in unit_symbol_lut if unit_symbol_lut[u][4]]
+
+        if symbol_wo_pref in unit_symbol_lut and symbol_wo_pref in prefixable_units:
+            return possible_prefix, symbol_wo_pref
+    return "", symbol_str
+
+
 def _lookup_unit_symbol(symbol_str, unit_symbol_lut):
     """
     Searches for the unit data tuple corresponding to the given symbol.
@@ -263,62 +285,42 @@ def _lookup_unit_symbol(symbol_str, unit_symbol_lut):
         return unit_symbol_lut[symbol_str]
 
     # could still be a known symbol with a prefix
-    possible_prefix = symbol_str[0]
-
-    if symbol_str[:2] == "da":
-        possible_prefix = "da"
-
-    if possible_prefix in unit_prefixes:
-        # the first character could be a prefix, check the rest of the symbol
-        symbol_wo_pref = symbol_str[1:]
+    prefix, symbol_wo_prefix = _split_prefix(symbol_str, unit_symbol_lut)
 
-        # deca is the only prefix with length 2
-        if symbol_str[:2] == "da":
-            symbol_wo_pref = symbol_str[2:]
-            possible_prefix = "da"
-
-        prefixable_units = [u for u in unit_symbol_lut if unit_symbol_lut[u][4]]
+    if prefix:
+        # lookup successful, it's a symbol with a prefix
+        unit_data = unit_symbol_lut[symbol_wo_prefix]
+        prefix_value = unit_prefixes[prefix]
 
-        unit_is_si_prefixable = (
-            symbol_wo_pref in unit_symbol_lut and symbol_wo_pref in prefixable_units
-        )
-
-        if unit_is_si_prefixable is True:
-            # lookup successful, it's a symbol with a prefix
-            unit_data = unit_symbol_lut[symbol_wo_pref]
-            prefix_value = unit_prefixes[possible_prefix]
-
-            if possible_prefix in latex_prefixes:
-                latex_repr = symbol_str.replace(
-                    possible_prefix, "{" + latex_prefixes[possible_prefix] + "}"
-                )
+        if prefix in latex_prefixes:
+            latex_repr = symbol_str.replace(prefix, "{" + latex_prefixes[prefix] + "}")
+        else:
+            # Need to add some special handling for comoving units
+            # this is fine for now, but it wouldn't work for a general
+            # unit that has an arbitrary LaTeX representation
+            if symbol_wo_prefix != "cm" and symbol_wo_prefix.endswith("cm"):
+                sub_symbol_wo_prefix = symbol_wo_prefix[:-2]
+                sub_symbol_str = symbol_str[:-2]
             else:
-                # Need to add some special handling for comoving units
-                # this is fine for now, but it wouldn't work for a general
-                # unit that has an arbitrary LaTeX representation
-                if symbol_wo_pref != "cm" and symbol_wo_pref.endswith("cm"):
-                    sub_symbol_wo_prefix = symbol_wo_pref[:-2]
-                    sub_symbol_str = symbol_str[:-2]
-                else:
-                    sub_symbol_wo_prefix = symbol_wo_pref
-                    sub_symbol_str = symbol_str
-                latex_repr = unit_data[3].replace(
-                    "{" + sub_symbol_wo_prefix + "}", "{" + sub_symbol_str + "}"
-                )
-
-            # Leave offset and dimensions the same, but adjust scale factor and
-            # LaTeX representation
-            ret = (
-                unit_data[0] * prefix_value,
-                unit_data[1],
-                unit_data[2],
-                latex_repr,
-                False,
+                sub_symbol_wo_prefix = symbol_wo_prefix
+                sub_symbol_str = symbol_str
+            latex_repr = unit_data[3].replace(
+                "{" + sub_symbol_wo_prefix + "}", "{" + sub_symbol_str + "}"
             )
 
-            unit_symbol_lut[symbol_str] = ret
+        # Leave offset and dimensions the same, but adjust scale factor and
+        # LaTeX representation
+        ret = (
+            unit_data[0] * prefix_value,
+            unit_data[1],
+            unit_data[2],
+            latex_repr,
+            False,
+        )
 
-            return ret
+        unit_symbol_lut[symbol_str] = ret
+
+        return ret
 
     # no dice
     raise UnitParseError(


Test patch:
diff --git a/unyt/tests/test_units.py b/unyt/tests/test_units.py
index d796e64..bdb7019 100644
--- a/unyt/tests/test_units.py
+++ b/unyt/tests/test_units.py
@@ -607,7 +607,18 @@ def test_code_unit():
 
     u = Unit("code_magnetic_field", registry=ureg)
     assert u.get_base_equivalent("mks") == Unit("T")
-    assert u.get_base_equivalent("cgs") == Unit("gauss")
+    with pytest.raises(UnitsNotReducible):
+        assert u.get_base_equivalent("cgs")
+
+    # see issue #60
+    u = Unit("s/m")
+    assert u.get_mks_equivalent() == Unit("s/m")
+    assert u.get_mks_equivalent() != Unit("ohm")
+    assert u.get_cgs_equivalent() == Unit("s/cm")
+
+    u = Unit("kC")
+    assert u.get_cgs_equivalent() == Unit("kesu")
+    assert u.get_cgs_equivalent().get_mks_equivalent() == u
 
     UnitSystem(ureg.unit_system_id, "code_length", "kg", "s", registry=ureg)