Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

base units deprecation and definitions #16658

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
5 participants
@smichr
Copy link
Member

commented Apr 15, 2019

References to other Issues or PRs

fixes #15647
closes #16651 if we can agree on this

Brief description of what is fixed or changed

  • All SI base units have a scale factor of 1
  • convert_to can accept a UnitSystem to supply the target units.

Other comments

When looking for how to access base units I found that there was no public facing method, nor was there a method to convert a quantity in one system to the units in another system (without having to specify the units but by only giving the system). There is a deprecated function for this purpose, however, so I added the ability to use a UnitSystem with convert_to

>>> convert_to(inch, MKS)
127*meter/5000

When looking at the deprecated test, however, I saw that the factor of 1000 was appearing in the conversion. I am not sure this is right.

I understand that base units need not have a scale factor of 1 -- e.g. the natural system includes c as the base unit, but it seems this test is written with the assumption that the scale factors are 1. If this is not the right assumption it would be good to hear an explanation of what someone trying to create such a system should have known and how they should have done it correctly.

This PR is offered with bias that scale_factor --namely, the assumption that base units have a scale factor of 1 -- is perhaps more significant than suggested in the discussion at #15647 by myself and others.

Release Notes

  • physics.units
    • convert_to will accept a UnitSystem which will supply the target units
    • all base units of the SI system have a scale factor of 1 (though this is not a general requirement of UnitSystems)
@sympy-bot

This comment has been minimized.

Copy link

commented Apr 15, 2019

Hi, I am the SymPy bot (v147). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

  • physics.units
    • convert_to will accept a UnitSystem which will supply the target units (#16658 by @smichr)

    • all base units of the SI system have a scale factor of 1 (though this is not a general requirement of UnitSystems) (#16658 by @smichr)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->

fixes #15647
closes #16651 if we can agree on this

#### Brief description of what is fixed or changed

* All SI base units have a scale factor of 1
* `convert_to` can accept a UnitSystem to supply the target units.

#### Other comments

When looking for how to access base units I found that there was no public facing method, nor was there a method to convert a quantity in one system to the units in another system (without having to specify the units but by only giving the system). There is a deprecated function for this purpose, however, so I added the ability to use a UnitSystem with `convert_to`
```python
>>> convert_to(inch, MKS)
127*meter/5000
```

When looking at the deprecated test, however, I saw that the factor of 1000 was appearing in the conversion. I am not sure this is right.

I understand that base units need not have a scale factor of 1 -- e.g. the `natural` system includes `c` as the base unit, but it seems this test is written with the assumption that the scale factors are 1. If this is not the right assumption it would be good to hear an explanation of what someone trying to create such a system should have known and how they should have done it correctly.

This PR is offered with bias that `scale_factor` --namely, the assumption that base units have a scale factor of 1 -- is perhaps more significant than suggested in the discussion at #15647 by myself and others.

#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* physics.units
    - `convert_to` will accept a UnitSystem which will supply the target units
    - all base units of the SI system have a scale factor of 1 (though this is not a general requirement of UnitSystems)
<!-- END RELEASE NOTES -->

@asmeurer asmeurer requested a review from Upabjojr Apr 15, 2019

@smichr smichr closed this Apr 15, 2019

@smichr smichr reopened this Apr 15, 2019

all base units have scale of 1 in SI
This is not a general requirement, but it is the
probably expectation for anyone using SI or extending
it (as the mksa example demonstrates).

@smichr smichr force-pushed the smichr:c2 branch from 1f2aaa1 to 92645c4 Apr 15, 2019

@smichr

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

@Upabjojr , I can see in the codebase how MKSA is created. Could you comment on the creation of mksa:

it would be good to hear an explanation of what someone trying to create such a system [as mksa] should have known and how they should have done it correctly.

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

I would like to discourage edits to the units module until the dimensions have been fixed.

My idea is the following: fix the scale factors between units when it is obvious that they will always have the same dimension (gram and kilogram). Other units should have no scale factor defined in the definitions file, their relations to units of the same dimension needs to be defined in the unit system instance.

These relations need some kind of advanced inheritance (e.g. SI inherits MKSA).

But most importantly, we need to fix the dimension system classes to be usable.

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

I would recommend not to add feature until the dimension system class has been refactored.

Anyway, what about CGS? If you call convert_to(.. , cgs) do you get the correct results? Don't remember if we have it implemented.

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

@Upabjojr , I can see in the codebase how MKSA is created. Could you comment on the creation of mksa:

it would be good to hear an explanation of what someone trying to create such a system [as mksa] should have known and how they should have done it correctly.

The existing units system classes should be reviewed. We don't currently handle dimension systems correctly.

For example, the speed of light is dimensionless in natural units, while it has dimension of velocity in SI. Even worse for electromagnetic quantities.

If I recall correctly, convert_to( ) is unable to handle dimensional contractions.

I would recommend not to merge this PR, convert_to(..., unit_system) is a lot more complicated to implement than this PR lets imagine.

@smichr

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

For example, the speed of light is dimensionless in natural units, while it has dimension of velocity in SI.
In this PR

>>> v = m/s
>>> convert_to(v, MKS)
meter/second
>>> cgs=UnitSystem((cm, g, s))
>>> convert_to(v, cgs)
100*centimeter/second
>>> convert_to(speed_of_light, MKS)
299792458*meter/second
>>> convert_to(speed_of_light, natural)
speed_of_light

In master you can't use UnitSystems but using the units still works

>>> convert_to(v, (m,kg,s))
meter/second
>>> convert_to(v, (cm,g,s))
100*centimeter/second

Even worse for electromagnetic quantities.

can you give an example?

If I recall correctly, convert_to( ) is unable to handle dimensional contractions.

can you give an example?

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

What I suggest should be done:

  1. do not set any dimension in definitions.py, just set the scale factors of quantities that are obviously scaled w.r.t. one another. Quantities of the same dimension can be deduced by interpreting the global scale factor as an equivalence class. The equivalence class represents units/quantities associated to the same dimension.
  2. Refactor the DimensionSystem class. Every dimension system object should define a list of dimensions and map at least one unit/quantity to a dimension for each equivalence class defined in point 1. If two units belonging two different equivalence classes of point 1 are mapped to the same dimension, then the whole equivalence class should be united inside the dimension system object.
  3. Refactor UnitSystem class. The unit system object should be required to provide scale factors between at least two units/quantities belonging two different equivalence classes defined in point 1 that have been united in point 2.
  4. Require the user to specify the unit system. Do not assume SI by default. convert_to(...) should only convert between units/quantities inside the same equivalence class under the currently adopted unit system.
  5. More complicated conversion tools to cross convert between unit systems using different equivalence classes of dimensions should be introduced. For example, contraction should use an equation to unite two different units (e.g. setting the speed of light equal to one identifies time as equivalent to space and leads to the common unit systems used in special relativity).

@smichr as you can see it's not an easy task to implement this module correctly. This is also one of the reasons I closed most of PR trying to edit the units module. I fear that adding more naive feature will simply make the refactory work harder.

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

can you give an example?

How should convert_to(ampere, cgs) work?

Using the master branch:

In [4]: convert_to(ampere, [centimeter, gram, second])                          
Out[4]: ampere

as ampere is detected to be independent of the three units (as it is under SI).

There is no special unit for current in CGS. One ampere is representable as combinations of cm, g, s.

@codecov

This comment has been minimized.

Copy link

commented Apr 15, 2019

Codecov Report

Merging #16658 into master will increase coverage by 0.048%.
The diff coverage is 100%.

@@              Coverage Diff              @@
##            master    #16658       +/-   ##
=============================================
+ Coverage   73.813%   73.862%   +0.048%     
=============================================
  Files          619       619               
  Lines       159227    159227               
  Branches     37370     37371        +1     
=============================================
+ Hits        117531    117609       +78     
+ Misses       36262     36189       -73     
+ Partials      5434      5429        -5
@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Actually convert_to(ampere, cgs) is possible, while the resulting expression can't be converted back to SI.

@smichr

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Although this would be better handled as a lookup in the given UnitSystem instead of clobbering all in one dict, this might be a direction that could be taken:

diff --cc sympy/physics/units/util.py
index 77b12e0,3a8bcb9..0000000
--- a/sympy/physics/units/util.py
+++ b/sympy/physics/units/util.py
@@@ -10,7 -10,7 +10,8 @@@ from sympy import Add, Mul, Pow, Tuple
  from sympy.core.compatibility import reduce, Iterable, ordered
  from sympy.physics.units.dimensions import Dimension, dimsys_default
  from sympy.physics.units.prefixes import Prefix
--from sympy.physics.units.quantities import Quantity
++from sympy.physics.units.quantities import Quantity, _derived
++from sympy.physics.units.unitsystem import UnitSystem
  from sympy.utilities.iterables import sift
  
  
@@@ -100,13 -100,13 +101,18 @@@ def convert_to(expr, target_units)
      7.62950196312651e-20*gravitational_constant**(-0.5)*hbar**0.5*speed_of_light**0.5
  
      """
-     from sympy.physics.units import UnitSystem
 -    from sympy.physics.units.quantities import _derived
 -
 -    expr = _derived.get(expr, expr)
 +    if isinstance(target_units, UnitSystem):
 +        target_units = target_units._base_units
  
      if not isinstance(target_units, (Iterable, Tuple)):
          target_units = [target_units]
  
++    if expr in target_units:
++        return expr  # or 1.*expr?
++
++    if all(isinstance(i, Quantity) for i in target_units):
++        expr = _derived.get(expr, expr)
++
      if isinstance(expr, Add):
          return Add.fromiter(convert_to(i, target_units) for i in expr.args)
  
diff --git a/sympy/physics/units/definitions.py b/sympy/physics/units/definitions.py
index 61e45e2..1e3185d 100644
--- a/sympy/physics/units/definitions.py
+++ b/sympy/physics/units/definitions.py
@@ -112,7 +112,7 @@
 
 coulomb = coulombs = C = Quantity("coulomb", abbrev='C')
 coulomb.set_dimension(charge)
-coulomb.set_scale_factor(One)
+coulomb.set_scale_factor(ampere*second)
 
 volt = volts = v = V = Quantity("volt", abbrev='V')
 volt.set_dimension(voltage)
diff --git a/sympy/physics/units/quantities.py b/sympy/physics/units/quantities.py
index b62ca26..82f2170 100644
--- a/sympy/physics/units/quantities.py
+++ b/sympy/physics/units/quantities.py
@@ -110,10 +110,11 @@ def set_scale_factor(self, scale_factor, unit_system="SI"):
             raise NotImplementedError("Currently only SI is supported")
 
         scale_factor = sympify(scale_factor)
-        if not scale_factor.is_Number:
-            _derived[self] = scale_factor
         # replace all prefixes by their ratio to canonical units:
         scale_factor = scale_factor.replace(lambda x: isinstance(x, Prefix), lambda x: x.scale_factor)
+        # store as a derived quantity
+        if not scale_factor.is_Number:
+            _derived[self] = scale_factor
         # replace all quantities by their ratio to canonical units:
         scale_factor = scale_factor.replace(lambda x: isinstance(x, Quantity), lambda x: x.scale_factor)
         Quantity.SI_quantity_scale_factors[self] = scale_factor
diff --git a/sympy/physics/units/tests/test_quantities.py b/sympy/physics/units/tests/test_quantities.py
index 553e70c..9e7a7d8 100644
--- a/sympy/physics/units/tests/test_quantities.py
+++ b/sympy/physics/units/tests/test_quantities.py
@@ -37,9 +37,9 @@ def test_convert_to():
     # assert (2*speed_of_light).convert_to(m / s) == 2 * 299792458 * m / s
     assert day.convert_to(s) == 86400*s
 
-    # Wrong dimension to convert:
+    # partial dimensions given
     assert q.convert_to(s) == q
-    assert speed_of_light.convert_to(m) == speed_of_light
+    assert speed_of_light.convert_to(m) == 299792458*meter/second
 
 
 def test_Quantity_definition():
diff --git a/sympy/physics/units/tests/test_util.py b/sympy/physics/units/tests/test_util.py
index eb0b333..1c5e104 100644
--- a/sympy/physics/units/tests/test_util.py
+++ b/sympy/physics/units/tests/test_util.py
@@ -6,7 +6,7 @@
     G, centimeter, coulomb, day, degree, gram, hbar, hour, inch, joule, kelvin,
     kilogram, kilometer, length, meter, mile, minute, newton, planck,
     planck_length, planck_mass, planck_temperature, planck_time, radians,
-    second, speed_of_light, steradian, time, km)
+    second, speed_of_light, steradian, time, m, km, kg, A, V, s)
 from sympy.physics.units.dimensions import dimsys_default
 from sympy.physics.units.systems import MKS
 from sympy.physics.units.util import convert_to, dim_simplify, check_dimensions
@@ -91,6 +91,8 @@ def test_convert_to_quantities():
     assert convert_to(pi, degree) == 180*degree
 
     assert convert_to(inch, MKS) == 127*meter/5000
+    assert convert_to(V, MKS) == kg*m**2/(A*s**3)
+    assert convert_to(coulomb, MKS) == A*s
 
 
 def test_convert_to_tuples_of_quantities():
@@ -103,7 +105,7 @@ def test_convert_to_tuples_of_quantities():
     assert convert_to(speed_of_light / 2, [meter, second, kilogram]) == meter/second*299792458 / 2
     # This doesn't make physically sense, but let's keep it as a conversion test:
     assert convert_to(2 * speed_of_light, [meter, second, kilogram]) == 2 * 299792458 * meter / second
-    assert convert_to(G, [G, speed_of_light, planck]) == 1.0*G
+    assert convert_to(G, [G, speed_of_light, planck]) == G
 
     assert NS(convert_to(meter, [G, speed_of_light, hbar]), n=7) == '6.187242e+34*gravitational_constant**0.5000000*hbar**0.5000000*speed_of_light**(-1.500000)'
     assert NS(convert_to(planck_mass, kilogram), n=7) == '2.176471e-8*kilogram'
@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

Although this would be better handled as a lookup in the given UnitSystem instead of clobbering all in one dict, this might be a direction that could be taken

ampere is always defined as coulomb*second, that is probably a universal definition and could be added to definitions.py.

We should abolish .set_dimension, that is definitely wrong.

@smichr smichr closed this May 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.