Improved sub-routine for string `quantity` arguments for find_unit #13875

Shikhar1998 · 2018-01-09T17:23:59Z

Re-written sub-routine for string quantity arguments for find_unit.

References to other Issues or PRs

Brief description of what is fixed or changed

The input element is searched with all the elements of units dictionary and matched for theirdimensions.

Other comments

Moreover, in PR #13438 problem of multiple instances of plank_length and plank_charge in resultant list for find_unit('charge') and find_unit('length') was raised.

Example:

>>> u.find_unit('charge')
['C', 'coulomb', 'coulombs', 'planck_charge', 'planck_charge']

To counter this problem, set was used. However, there is no special need for it, as all the definitions are inherently unique. This problem has also been eliminated.

Re-written sub-routine for string quantity arguments for find_unit.

Now covers all possible cases

ylemkimon

+1

ylemkimon · 2018-01-09T18:40:15Z

sympy/physics/units/__init__.py

@@ -222,23 +222,28 @@ def find_unit(quantity):



I think docstring should explain its exact behavior

@ylemkimon, I have made the requested changes, please have a look.

I think all instances of units/dimensions with these arguments. is little ambiguous.
If a string is given, it currently returns
(i) all units matching the string
(ii) if a dimension exactly matching the string exists, all units with that dimension
(iii) if a unit exactly matching the string exists, all units with same dimension

Explained the result for possible 2 input cases: 1. String and 2. Quantity/Dimension

72 character rule

Shikhar1998 · 2018-01-12T06:16:24Z

@smichr, can you please review this PR.

ylemkimon · 2018-01-12T07:14:02Z

sympy/physics/units/__init__.py

+        if isinstance(dim, Quantity): dim = dim.dimension
+        for i in dir(u):
+            quant_attr = getattr(u, i)
+            if quantity in i or (isinstance(quant_attr, Quantity) and quant_attr.dimension == dim):


I don't think it should return Dimensions.

True, as name of function find_unit itself implies to find all instances of that unit. However, original docstring states that function should return all Dimensions.

A separate function like find_dimension can be added to return all the dimensions. Please suggest.

However, the results of these functions (find_unit and find_dimension) would be quite alike, which may make this exercise quite dispensable.

Shikhar1998 · 2018-01-12T18:01:53Z

Done!! ping @ylemkimon .

Shikhar1998 · 2018-01-17T11:32:53Z

@smichr, as you were associated with PR #13438 can you please give some comments on the current changes. Thanks!!

Shikhar1998 · 2018-01-20T05:55:16Z

@ylemkimon, ping.

ylemkimon · 2018-01-20T06:30:59Z

I think documentation needs improvement, but I'm not a native English speaker, so I cannot think of better one.

Shikhar1998 · 2018-01-20T07:49:58Z

@asmeurer, do you have some suggestions to improve the documentation.

smichr · 2018-01-23T10:46:29Z

sympy/physics/units/__init__.py

@@ -210,12 +210,18 @@

 def find_unit(quantity):
    """
-    Return a list of matching units or dimension names.


Perhaps it would be clearer to just demonstrate by example rather than to write an abstract statement which will be hard to understand unless you already understand it or until you see an example:

""" Return the list of units that match the given quantity. Parameters ========== quantity : string, Dimension, or Quantity Examples ======== >>> from sympy.physics import units as u When the quantity is a unit given is a string or unit, ... >>> u.find('charge') # unit as string >>> u.find(u.charge) # unit as unit When the quantity given is a Dimension, ...

Quite a nice idea, thanks for the insight. Made changes, please check.

@smichr

Made changes as suggested by @smichr

smichr · 2018-01-23T15:59:47Z

sympy/physics/units/__init__.py

    >>> u.find_unit(u.inch**3)[:5]
    ['l', 'cl', 'dl', 'ml', 'liter']
+
+    # When the input is a string, it returns units which have input


Two comments:

go ahead and write the comments without the leading #

the last comment about using strings is a bit confusing, perhaps
because of 'sub-script'. I think "substring" would be more recognized in this context. Myabe

When the input is a string, units or Quantities are returned. The units will have the input as a substring; Quantities will a dimension that matches the input.

But if that is right, I'm not sure why the behavior of trying to match dimensions was added. That can already be done with u.dimension. string input is used when you aren't sure of the spelling or quantity that you are looking for; once you find it you can then use the corresponding dimension in your next search. Is there an unecessary complication by combining both outputs for a string input? e.g. if you can't do [u.find_unit(gettattr(u, i)) for i in u.find_unit('foo')] ... or soemthing like that ... then this is maybe not a good direction to go.

That can already be done with u.dimension. string input is used when you aren't sure of the spelling or quantity that you are looking for; once you find it you can then use the corresponding dimension in your next search.

Hmm.., I have a simple idea for this. We can match the various quantities in the dictionary u for given string and give outputs like this:

>>> u.find_unit('char') do you mean: charge # for incomplete entries >>> u.find_unit('charge') 1 # a perfect match >>> u.find_unit('0x00x0') 0 # no match OR simply an array of suitable matches ['charge']

smichr · 2018-01-26T18:47:41Z

What do you think about these changes:

diff --git a/sympy/physics/units/__init__.py b/sympy/physics/units/__init__.py
index 75de4ee..dc38476 100644
--- a/sympy/physics/units/__init__.py
+++ b/sympy/physics/units/__init__.py
@@ -249,29 +249,38 @@ def find_unit(quantity):
 
     """
     import sympy.physics.units as u
-    rv = []
+    rv = set()
+    IA = [(i, getattr(u, i)) for i in dir(u)]
+    IA = [(i, a) for i, a in IA if isinstance(a, (Quantity, Dimension))]
     if isinstance(quantity, string_types):
-        dim = getattr(u, quantity)
-        if isinstance(dim, Quantity): dim = dim.dimension
-        for i in dir(u):
-            quant_attr = getattr(u, i)
-            if quantity in i or (isinstance(quant_attr, Quantity) and quant_attr.dimension == dim):
-                if not isinstance(quant_attr, Dimension): rv.append(i)
-
+        dim = getattr(u, quantity, None)
+        if dim is None:
+            for i, a in IA:
+                if quantity in i:
+                    rv.add(i)
+                    rv.update(find_unit(i))
+        else:
+            rv.update([i for i, _ in IA if quantity in i])
+            if isinstance(dim, Quantity):
+                dim = dim.dimension
+            for i, quant_attr in IA:
+                if quantity in i or (isinstance(quant_attr, Quantity)
+                        and quant_attr.dimension == dim):
+                    if not isinstance(quant_attr, Dimension):
+                        rv.add(i)
     else:
-        for i in sorted(dir(u)):
-            other = getattr(u, i)
-            if not isinstance(other, Quantity):
-                continue
+        for i, other in IA:
             if isinstance(quantity, Quantity):
-                if quantity.dimension == other.dimension:
-                    rv.append(str(i))
+                if isinstance(other, Quantity) and quantity.dimension == other.dimension:
+                    rv.add(i)
             elif isinstance(quantity, Dimension):
-                if other.dimension == quantity:
-                    rv.append(str(i))
-            elif other.dimension == Dimension(Quantity.get_dimensional_expr(quantity)):
-                rv.append(str(i))
-    return sorted(rv, key=len)
+                if isinstance(other, Quantity) and other.dimension == quantity:
+                    rv.add(i)
+            elif isinstance(other, Quantity) and other.dimension == Dimension(Quantity.get_dimensional_expr(quantity)):
+                rv.add(i)
+    if not rv:
+        return []
+    return list(list(zip(*sorted(zip([len(i) for i in rv], rv))))[1])
 
 # NOTE: the old units module had additional variables:
 # 'density', 'illuminance', 'resistance'.
diff --git a/sympy/physics/units/tests/test_quantities.py b/sympy/physics/units/tests/test_quantities.py
index fd9f6b0..94bda98 100644
--- a/sympy/physics/units/tests/test_quantities.py
+++ b/sympy/physics/units/tests/test_quantities.py
@@ -221,6 +221,8 @@ def test_find_unit():
     assert find_unit('coulomb') == ['C', 'coulomb', 'coulombs',
                                     'planck_charge', 'coulomb_constant']
     assert find_unit(coulomb) == ['C', 'coulomb', 'coulombs', 'planck_charge']
+    assert find_unit('char') == ['C', 'charge', 'coulomb', 'coulombs', 'planck_charge']
+    assert find_unit('charge') == ['C', 'charge', 'coulomb', 'coulombs', 'planck_charge']
     assert find_unit(charge) == ['C', 'coulomb', 'coulombs', 'planck_charge']
     assert find_unit(inch) == [
         'm', 'au', 'cm', 'dm', 'ft', 'km', 'ly', 'mi', 'mm', 'nm', 'pm', 'um',
@@ -237,7 +239,7 @@ def test_find_unit():
         'l', 'cl', 'dl', 'ml', 'liter', 'quart', 'liters', 'quarts',
         'deciliter', 'centiliter', 'deciliters', 'milliliter',
         'centiliters', 'milliliters', 'planck_volume']
-    assert find_unit('voltage') == ['V', 'v', 'volt', 'volts', 'planck_voltage']
+    assert find_unit('voltage') == ['V', 'v', 'volt', 'volts', 'voltage', 'planck_voltage']
 
 
 def test_Quantity_derivative():

@smichr

Changes made as suggested by @smichr

Shikhar1998 · 2018-01-27T05:12:53Z

@smichr, Thank's for the valuable input. I have committed these suggested changes. I had one question regarding the current implementation; when u.find_unit('') is executed, it returns the entire units dictionary. Is this a valid behavior or not?

ylemkimon · 2018-01-27T09:47:46Z

@smichr

IA = [(i, a) for i, a in IA if isinstance(a, (Quantity, Dimension))]

I still think Dimensions should not be included in the return value of find_unit.

smichr · 2018-01-27T22:14:35Z

I still think Dimensions should not be included in the return value of find_unit.

Can you give an example of what the output is that you would like it not to be?

smichr · 2018-01-28T14:18:30Z

Is this a valid behavior or not?

probably not...we could just special case that and return [].

Now returns an empty list

smichr · 2018-01-29T15:09:31Z

@ylemkimon , why don't you think Dimensions should be included in the return result?

As it is now, the original intent of the docstring should be restored to indicate that dimensions may be returned, too. Otherwise, if there is a good reason not to, then only the code needs to be modified and the docstring left alone.

smichr · 2018-01-31T11:11:37Z

Would it make sense to return dimensions if the quantity is ''?

--- a/sympy/physics/units/__init__.py
+++ b/sympy/physics/units/__init__.py
@@ -249,34 +249,27 @@ def find_unit(quantity):

     """
     import sympy.physics.units as u
+    IQ = [(i, getattr(u, i)) for i in dir(u)]
+    if not quantity:
+        return sorted([(i, q) for i, q in IQ if isinstance(q, Dimension)])
+    IQ = [(i, q) for i, q in IQ if isinstance(q, Quantity)]
     rv = set()
-    IA = [(i, getattr(u, i)) for i in dir(u)]
-    IA = [(i, a) for i, a in IA if isinstance(a, (Quantity, Dimension))]
     if isinstance(quantity, string_types):
+        rv.update([i for i, _ in IQ if quantity in i])
         dim = getattr(u, quantity, None)
-        if dim is None:
-            for i, a in IA:
-                if quantity in i:
-                    rv.add(i)
-                    rv.update(find_unit(i))
-        else:
-            rv.update([i for i, _ in IA if quantity in i])
+        if dim is not None:
             if isinstance(dim, Quantity):
                 dim = dim.dimension
-            for i, quant_attr in IA:
-                if quantity in i or (isinstance(quant_attr, Quantity)
-                        and quant_attr.dimension == dim):
-                    if not isinstance(quant_attr, Dimension):
-                        rv.add(i)
-    else:
-        for i, other in IA:
-            if isinstance(quantity, Quantity):
-                if isinstance(other, Quantity) and quantity.dimension == other.dimension:
+            for i, quant in IQ:
+                if dim == quant.dimension:
                     rv.add(i)
-            elif isinstance(quantity, Dimension):
-                if isinstance(other, Quantity) and other.dimension == quantity:
-                    rv.add(i)
-            elif isinstance(other, Quantity) and other.dimension == Dimension(Quantity.get_dimensional_expr(quantity)):
+    else:
+        if isinstance(quantity, Quantity):
+            dim = quantity.dimension
+        else:
+            dim = Dimension(Quantity.get_dimensional_expr(quantity))
+        for i, quant in IQ:
+            if dim == quant.dimension:
                 rv.add(i)
     if not rv:
         return []

ylemkimon · 2018-01-31T11:15:29Z

@smichr Dimension is a dimension of units, rather than a unit, and different class to Quantity. The user would expect to perform Quantity methods on units returned by find_unit.

smichr · 2018-02-01T02:17:40Z

@ylemkimon , here's an example of how someone might use the null find to get the dimensions and then use them for a further search:

>>> u.find_unit('')
['acceleration', 'action', 'amount', 'amount_of_substance', 'capacitance', 'char
ge', 'conductance', 'current', 'energy', 'force', 'frequency', 'impedance', 'ind
uctance', 'length', 'luminosity', 'luminous_intensity', 'magnetic_density', 'mag
netic_flux', 'magnetic_flux_density', 'mass', 'momentum', 'power', 'pressure', '
speed', 'temperature', 'time', 'velocity', 'voltage', 'volume']

>>> u.find_unit(u.length/u.time)
['sr', 'deg', 'mil', 'rad', 'degree', 'radian', 'degrees', 'percent', 'radians',
 'percents', 'permille', 'steradian', 'steradians', 'angular_mil', 'angular_mils
', 'avogadro_number']

...not sure why avogadro_number is showing up, though.

Shikhar1998 · 2018-02-01T02:37:43Z

@ylemkimon, will it be fine to rename the function as find_unit_dimension(). I think this will the eliminate the possibility of any unnecessary confusion in future.

smichr · 2018-02-01T09:22:51Z

rename the function as find_unit_dimension

Or a function named dimensions that simply returns the list sorted list of dimensions.

czgdp1807 · 2019-09-16T17:57:37Z

@Upabjojr Is this PR having a chance to be merged?

czgdp1807 · 2020-01-03T17:39:32Z

The PR seems to be a valid one. @Shikhar1998 Would you still like to continue your work on this? Thanks for your contributions.

Fixes find_unit

110b02f

Re-written sub-routine for string quantity arguments for find_unit.

Shikhar1998 changed the title ~~Re-written sub-routine for string quantity arguments for find_unit.~~ Improved sub-routine for string quantity arguments for find_unit Jan 9, 2018

Some changes in tests

2cc9a7e

Now covers all possible cases

ylemkimon requested changes Jan 9, 2018

View reviewed changes

Shikhar1998 added 2 commits January 10, 2018 17:29

Improved Documentation

79669d0

Explained the result for possible 2 input cases: 1. String and 2. Quantity/Dimension

Formatting Docstring

fc77d8a

72 character rule

ylemkimon reviewed Jan 12, 2018

View reviewed changes

Removed definations from output

7dbc7a4

smichr reviewed Jan 23, 2018

View reviewed changes

Modifications in Documentation

15cf648

Made changes as suggested by @smichr

smichr reviewed Jan 23, 2018

View reviewed changes

Modifications in __init__.py

76b42f8

Changes made as suggested by @smichr

Handled case: quantity equal to empty string

aa91be3

Now returns an empty list

Abdullahjavednesar added the PR: author's turn The PR has been reviewed and the author needs to submit more changes. label Apr 4, 2018

oscarbenjamin added the physics.units Units and dimensions label Jan 24, 2019

czgdp1807 added the Stalled label Sep 16, 2019

czgdp1807 added Please take over and removed Stalled labels Mar 18, 2020

github-actions bot added the Merge conflict label Oct 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved sub-routine for string `quantity` arguments for find_unit #13875

Improved sub-routine for string `quantity` arguments for find_unit #13875

Shikhar1998 commented Jan 9, 2018

ylemkimon left a comment

ylemkimon Jan 9, 2018

Shikhar1998 Jan 10, 2018

ylemkimon Jan 12, 2018 •

edited

Shikhar1998 commented Jan 12, 2018

ylemkimon Jan 12, 2018

Shikhar1998 Jan 12, 2018

Shikhar1998 Jan 12, 2018

Shikhar1998 Jan 12, 2018

Shikhar1998 commented Jan 12, 2018

Shikhar1998 commented Jan 17, 2018 •

edited

Shikhar1998 commented Jan 20, 2018

ylemkimon commented Jan 20, 2018 •

edited

Shikhar1998 commented Jan 20, 2018 •

edited

smichr Jan 23, 2018

Shikhar1998 Jan 23, 2018

smichr Jan 23, 2018

Shikhar1998 Jan 23, 2018 •

edited

smichr commented Jan 26, 2018

Shikhar1998 commented Jan 27, 2018

ylemkimon commented Jan 27, 2018

smichr commented Jan 27, 2018

smichr commented Jan 28, 2018

smichr commented Jan 29, 2018

smichr commented Jan 31, 2018

ylemkimon commented Jan 31, 2018

smichr commented Feb 1, 2018

Shikhar1998 commented Feb 1, 2018

smichr commented Feb 1, 2018

czgdp1807 commented Sep 16, 2019

czgdp1807 commented Jan 3, 2020

Improved sub-routine for string quantity arguments for find_unit #13875

Are you sure you want to change the base?

Improved sub-routine for string quantity arguments for find_unit #13875

Conversation

Shikhar1998 commented Jan 9, 2018

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

ylemkimon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylemkimon Jan 12, 2018 • edited

Choose a reason for hiding this comment

Shikhar1998 commented Jan 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shikhar1998 commented Jan 12, 2018

Shikhar1998 commented Jan 17, 2018 • edited

Shikhar1998 commented Jan 20, 2018

ylemkimon commented Jan 20, 2018 • edited

Shikhar1998 commented Jan 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shikhar1998 Jan 23, 2018 • edited

Choose a reason for hiding this comment

smichr commented Jan 26, 2018

Shikhar1998 commented Jan 27, 2018

ylemkimon commented Jan 27, 2018

smichr commented Jan 27, 2018

smichr commented Jan 28, 2018

smichr commented Jan 29, 2018

smichr commented Jan 31, 2018

ylemkimon commented Jan 31, 2018

smichr commented Feb 1, 2018

Shikhar1998 commented Feb 1, 2018

smichr commented Feb 1, 2018

czgdp1807 commented Sep 16, 2019

czgdp1807 commented Jan 3, 2020

Improved sub-routine for string `quantity` arguments for find_unit #13875

Improved sub-routine for string `quantity` arguments for find_unit #13875

ylemkimon Jan 12, 2018 •

edited

Shikhar1998 commented Jan 17, 2018 •

edited

ylemkimon commented Jan 20, 2018 •

edited

Shikhar1998 commented Jan 20, 2018 •

edited

Shikhar1998 Jan 23, 2018 •

edited