Repository: sympy/sympy
Pull request: physics/optics: Updated Medium to inherit from Basic
<!-- 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://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #22664


#### Brief description of what is fixed or changed
Changed `Medium` to inherit from `Basic` instead of `Symbol` and to store its arguments in `args` after checking if any arguments are `None`. This makes `Medium` hashable and also makes `sprepr` work correctly when called on a `Medium` object.

Also changed a condition in the `__new__` method of `Medium` that checks whether its arguments are consistent to not raise a `TypeError` due to `Relational` not having a well-defined truth value.


#### Other comments


#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers.

or if no release note(s) should be included use:

NO ENTRY

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.optics
  * changed Medium to inherit from Basic instead of Symbol
<!-- END RELEASE NOTES -->


Problem statement:
`Medium` not hashable
This shows up as an issue in the LaTeX-printing, but is probably an issue elsewhere as well.

```
from sympy.physics.optics import Medium
Medium('m')
```
gives
```
Traceback (most recent call last):

  File "C:\Users\Oscar\miniconda3\lib\site-packages\IPython\core\formatters.py", line 345, in __call__
    return method()

  File "C:\Users\Oscar\sympy\sympy\core\_print_helpers.py", line 64, in _repr_latex_
    s = latex(self, mode='plain')

  File "C:\Users\Oscar\sympy\sympy\printing\printer.py", line 373, in __call__
    return self.__wrapped__(*args, **kwargs)

  File "C:\Users\Oscar\sympy\sympy\printing\latex.py", line 3011, in latex
    return LatexPrinter(settings).doprint(expr)

  File "C:\Users\Oscar\sympy\sympy\printing\latex.py", line 254, in doprint
    tex = Printer.doprint(self, expr)

  File "C:\Users\Oscar\sympy\sympy\printing\printer.py", line 293, in doprint
    return self._str(self._print(expr))

  File "C:\Users\Oscar\sympy\sympy\printing\printer.py", line 332, in _print
    return printmethod(expr, **kwargs)

  File "C:\Users\Oscar\sympy\sympy\printing\latex.py", line 1569, in _print_Symbol
    if expr in self._settings['symbol_names']:

TypeError: unhashable type: 'Medium'
```

Hints from issue discussion:
It also needs a proper `srepr` method as any additional arguments are not returned.

```
In [10]: srepr(Medium('m', 3))
Out[10]: "Medium('m')"
```
The problem is a badly written `__new__` method:
https://github.com/sympy/sympy/blob/81cd2630631e315586f796646a7f9268e71bf484/sympy/physics/optics/medium.py#L71-L89

It's simply not storing its arguments in `_args`.

Actually, considering it is a `Symbol`, it shouldn't show (and store) any arguments and just print `m`.

The if statements also don't catch all possible inputs.
Could the issue also be because Medium does not properly override `__hash__`?. Creating a`__hash__` method as 
```
    def __hash__(self):
        return super().__hash__()
```
fixes the issue.
As an aside, how should `_hashable_content` be implemented for Medium, considering that `__eq__` only checks if the refractive indices are equal. I guess the hashable content should only include the refractive index so that `__hash__` and `__eq__` remain consistent. Am I right here? 
@ThePauliPrinciple it is indeed not really following the traditional design guidelines, but has been hanging around for eight years or so. (Still no-one seems to have tried to render a LaTeX for it...)

@NikhilSDate yes, that is one of the options. I agree with you that there should be some resemblance between `__hash__` and `__eq__`, but I am not in a position to say what is important for `Medium`. 

Still, probably the best way is to change `Medium` from inheriting `Symbol` to `Expr`(?) and then store all the arguments to make it a "proper" SymPy class. It may be that it solves the hash issue as well then.
What is more suprising is that this class can even exist. Since it is a subclass of `Basic`, it should be using `__slots__`, which should trigger an error of `__eq__` is defined but not `__hash__` as far as I am aware.

If it is converted to an `Expr`/`Basic` and stores all its arguments, there should be no reason to define `__eq__` (although less than/greater than might still be required).

It should then however be rewritten to not store it's arguments as `None`, which I think can currently happen. Using `_sympify` would also be preferable. (but means it can only happen after checking for `None`.
If there is a consensus that such a change is needed, I can go ahead and change `Medium` so that it inherits from `Expr` and stores its arguments in `_args`. Should I go ahead?

Commit message: removed check for empty name

Patch to review:
diff --git a/sympy/physics/optics/medium.py b/sympy/physics/optics/medium.py
index e47179c22bb0..204161864203 100644
--- a/sympy/physics/optics/medium.py
+++ b/sympy/physics/optics/medium.py
@@ -7,7 +7,8 @@
 
 __all__ = ['Medium']
 
-from sympy.core.symbol import Symbol
+from sympy.core.basic import Basic
+from sympy.core.symbol import Str
 from sympy.core.sympify import sympify
 from sympy.functions.elementary.miscellaneous import sqrt
 from sympy.physics.units import speed_of_light, u0, e0
@@ -18,7 +19,7 @@
 _u0mksa = u0.convert_to(meter*kilogram/(ampere**2*second**2))
 
 
-class Medium(Symbol):
+class Medium(Basic):
 
     """
     This class represents an optical medium. The prime reason to implement this is
@@ -69,23 +70,31 @@ class Medium(Symbol):
     """
 
     def __new__(cls, name, permittivity=None, permeability=None, n=None):
-        obj = super().__new__(cls, name)
-        obj._permittivity = sympify(permittivity)
-        obj._permeability = sympify(permeability)
-        obj._n = sympify(n)
+        if not isinstance(name, Str):
+            name = Str(name)
+
         if n is not None:
             if permittivity is not None and permeability is None:
-                obj._permeability = n**2/(c**2*obj._permittivity)
+                permeability = n**2/(c**2*permittivity)
             if permeability is not None and permittivity is None:
-                obj._permittivity = n**2/(c**2*obj._permeability)
+                permittivity = n**2/(c**2*permeability)
             if permittivity is not None and permittivity is not None:
-                if abs(n - c*sqrt(obj._permittivity*obj._permeability)) > 1e-6:
-                   raise ValueError("Values are not consistent.")
+                expr = abs(n - c*sqrt(permittivity*permeability))
+                expr = expr.subs({meter: 1, second: 1})
+                if len(expr.free_symbols) == 0 and expr > 1e-6:
+                    raise ValueError("Values are not consistent.")
         elif permittivity is not None and permeability is not None:
-            obj._n = c*sqrt(permittivity*permeability)
+            n = c*sqrt(permittivity*permeability)
         elif permittivity is None and permeability is None:
-            obj._permittivity = _e0mksa
-            obj._permeability = _u0mksa
+            permittivity = _e0mksa
+            permeability = _u0mksa
+            n = c*sqrt(permittivity*permeability)
+        args = list(map(sympify, (permittivity, permeability, n)))
+        obj = super().__new__(cls, name, *args)
+        obj.name = name
+        obj._permittivity = args[0]
+        obj._permeability = args[1]
+        obj._n = args[2]
         return obj
 
     @property


Code review comments:
diff_hunk: @@ -69,23 +70,32 @@ class Medium(Symbol):
     """
 
     def __new__(cls, name, permittivity=None, permeability=None, n=None):
-        obj = super().__new__(cls, name)
-        obj._permittivity = sympify(permittivity)
-        obj._permeability = sympify(permeability)
-        obj._n = sympify(n)
+
+        if not isinstance(name, Str):
+            name = Str(name)
+
         if n is not None:
             if permittivity is not None and permeability is None:
-                obj._permeability = n**2/(c**2*obj._permittivity)
+                permeability = n**2/(c**2*permittivity)
             if permeability is not None and permittivity is None:
-                obj._permittivity = n**2/(c**2*obj._permeability)
+                permittivity = n**2/(c**2*permeability)
             if permittivity is not None and permittivity is not None:
-                if abs(n - c*sqrt(obj._permittivity*obj._permeability)) > 1e-6:
-                   raise ValueError("Values are not consistent.")
+                expr = abs(n - c*sqrt(permittivity*permeability))
+                expr = expr.subs({meter: 1, second: 1})
+                if len(expr.free_symbols) == 0 and expr > 1e-6:
+                    raise ValueError("Values are not consistent.")
         elif permittivity is not None and permeability is not None:
-            obj._n = c*sqrt(permittivity*permeability)
+            n = c*sqrt(permittivity*permeability)
         elif permittivity is None and permeability is None:
-            obj._permittivity = _e0mksa
-            obj._permeability = _u0mksa
+            permittivity = _e0mksa
+            permeability = _u0mksa
+            n = c*sqrt(permittivity*permeability)
+        args = list(map(sympify, (permittivity, permeability, n)))
path: sympy/physics/optics/medium.py
text: @user1:
`_sympify` is prefered over `sympify` (it is stricter and shouldn't return non-Basic types, which `sympify` might)

@author:
Alright, I'll make the change.