From 5878c154d0a76c633a5c830ff11c99343ab21bfe Mon Sep 17 00:00:00 2001
From: mmatera <matera@fisica.unlp.edu.ar>
Date: Tue, 13 Sep 2022 15:10:51 -0300
Subject: [PATCH 1/4] This is one part of #551, plus several comments into the
 code.

* adding the prototype of to_python in BaseElement
* adding type annotations to its paramenters.
* removing the use of the parameter n_evaluation in ``expr.to_python(n_evaluation=evaluation)`` in favor of ``eval_N(expr, evaluation).to_python()``
* adding several comments.
---
 mathics/builtin/binary/io.py             | 69 +++++++++++++-----------
 mathics/builtin/box/graphics3d.py        |  7 ++-
 mathics/builtin/drawing/plot.py          | 10 ++--
 mathics/builtin/files_io/filesystem.py   |  8 +--
 mathics/builtin/numbers/numbertheory.py  | 21 ++++++--
 mathics/builtin/numbers/randomnumbers.py | 26 ++++++---
 mathics/core/convert/function.py         |  2 +-
 mathics/core/element.py                  | 18 ++++++-
 mathics/core/symbols.py                  | 38 ++++++++++---
 9 files changed, 138 insertions(+), 61 deletions(-)

diff --git a/mathics/builtin/binary/io.py b/mathics/builtin/binary/io.py
index 9e1e3229b..761b987d4 100644
--- a/mathics/builtin/binary/io.py
+++ b/mathics/builtin/binary/io.py
@@ -14,6 +14,7 @@
 from mathics.core.atoms import Complex, Integer, MachineReal, Real, String
 from mathics.core.convert.expression import to_expression, to_mathics_list
 from mathics.core.convert.mpmath import from_mpmath
+from mathics.core.evaluators import eval_N
 from mathics.core.expression import Expression
 
 from mathics.core.systemsymbols import (
@@ -941,6 +942,7 @@ def apply(self, name, n, b, typ, evaluation):
 
         # Write to stream
         i = 0
+        # TODO: please, modularize me.
         while i < len(pyb):
             x = pyb[i]
             # Types are "repeated as many times as necessary"
@@ -948,61 +950,68 @@ def apply(self, name, n, b, typ, evaluation):
 
             # Coerce x
             if t == "TerminatedString":
-                x = x.get_string_value() + "\x00"
+                x_py = x.get_string_value() + "\x00"
             elif t.startswith("Real"):
                 if isinstance(x, Real):
-                    x = x.to_python()
+                    x_py = x.to_python()
                 elif x.has_form("DirectedInfinity", 1):
                     if x.elements[0].get_int_value() == 1:
-                        x = float("+inf")
+                        x_py = float("+inf")
                     elif x.elements[0].get_int_value() == -1:
-                        x = float("-inf")
+                        x_py = float("-inf")
                     else:
-                        x = None
-                elif isinstance(x, Symbol) and x.get_name() == "System`Indeterminate":
-                    x = float("nan")
+                        x_py = None
+                elif x is SymbolIndeterminate:
+                    x_py = float("nan")
                 else:
-                    x = None
-                assert x is None or isinstance(x, float)
+                    x_py = None
+                assert x_py is None or isinstance(x_py, float)
             elif t.startswith("Complex"):
                 if isinstance(x, (Complex, Real, Integer)):
-                    x = x.to_python()
+                    x_py = x.to_python()
                 elif x.has_form("DirectedInfinity", 1):
-                    x = x.elements[0].to_python(n_evaluation=evaluation)
+                    x_py = eval_N(x.elements[0], evaluation).to_python()
 
                     # x*float('+inf') creates nan if x.real or x.imag are zero
-                    x = complex(
-                        x.real * float("+inf") if x.real != 0 else 0,
-                        x.imag * float("+inf") if x.imag != 0 else 0,
+                    x_py = complex(
+                        x_py.real * float("+inf") if x_py.real != 0 else 0,
+                        x_py.imag * float("+inf") if x_py.imag != 0 else 0,
                     )
-                elif isinstance(x, Symbol) and x.get_name() == "System`Indeterminate":
-                    x = complex(float("nan"), float("nan"))
+                elif x is SymbolIndeterminate:
+                    x_py = complex(float("nan"), float("nan"))
                 else:
-                    x = None
+                    x_py = None
             elif t.startswith("Character"):
                 if isinstance(x, Integer):
-                    x = [String(char) for char in str(x.get_int_value())]
-                    pyb = list(chain(pyb[:i], x, pyb[i + 1 :]))
+                    x_list = [String(char) for char in str(x.get_int_value())]
+                    pyb = list(chain(pyb[:i], x_list, pyb[i + 1 :]))
                     x = pyb[i]
-                if isinstance(x, String) and len(x.get_string_value()) > 1:
-                    x = [String(char) for char in x.get_string_value()]
-                    pyb = list(chain(pyb[:i], x, pyb[i + 1 :]))
+                    assert isinstance(x, String)
+                if isinstance(x, String) and len(x.value) > 1:
+                    x_list = [String(char) for char in x.value]
+                    pyb = list(chain(pyb[:i], x_list, pyb[i + 1 :]))
                     x = pyb[i]
-                x = x.get_string_value()
+                    assert isinstance(x, String)
+                    x_py = x.value
+                else:
+                    # Not sure what happens here...
+                    # TODO: Check...
+                    x_py = x.get_string_value()
             elif t == "Byte" and isinstance(x, String):
-                if len(x.get_string_value()) > 1:
-                    x = [String(char) for char in x.get_string_value()]
-                    pyb = list(chain(pyb[:i], x, pyb[i + 1 :]))
+                if len(x.value) > 1:
+                    x_list = [String(char) for char in x.value]
+                    pyb = list(chain(pyb[:i], x_list, pyb[i + 1 :]))
                     x = pyb[i]
-                x = ord(x.get_string_value())
+                assert isinstance(x, String)
+                x_py = ord(x.value)
             else:
-                x = x.get_int_value()
+                x_py = x.get_int_value()
 
-            if x is None:
+            if x_py is None:
                 return evaluation.message(SymbolBinaryWrite, "nocoerce", b)
 
             try:
-                self.writers[t](stream.io, x)
+                self.writers[t](stream.io, x_py)
             except struct.error:
                 return evaluation.message(SymbolBinaryWrite, "nocoerce", b)
             i += 1
diff --git a/mathics/builtin/box/graphics3d.py b/mathics/builtin/box/graphics3d.py
index 1411c85f5..646306aa3 100644
--- a/mathics/builtin/box/graphics3d.py
+++ b/mathics/builtin/box/graphics3d.py
@@ -25,8 +25,11 @@
 )
 
 from mathics.builtin.drawing.graphics_internals import get_class
-from mathics.core.symbols import Symbol, SymbolTrue
+
+
 from mathics.core.formatter import lookup_method
+from mathics.core.evaluators import eval_N
+from mathics.core.symbols import Symbol, SymbolTrue
 
 
 class Graphics3DBox(GraphicsBox):
@@ -182,7 +185,7 @@ def _prepare_elements(self, leaves, options, max_width=None):
 
         # ViewPoint Option
         viewpoint_option = self.graphics_options["System`ViewPoint"]
-        viewpoint = viewpoint_option.to_python(n_evaluation=evaluation)
+        viewpoint = eval_N(viewpoint_option, evaluation).to_python()
 
         if isinstance(viewpoint, list) and len(viewpoint) == 3:
             if all(isinstance(x, numbers.Real) for x in viewpoint):
diff --git a/mathics/builtin/drawing/plot.py b/mathics/builtin/drawing/plot.py
index f8bf95459..ef7c84e4b 100644
--- a/mathics/builtin/drawing/plot.py
+++ b/mathics/builtin/drawing/plot.py
@@ -464,7 +464,7 @@ def check_range(range):
             return False
 
         plotrange_option = self.get_option(options, "PlotRange", evaluation)
-        plotrange = plotrange_option.to_python(n_evaluation=evaluation)
+        plotrange = eval_N(plotrange_option, evaluation).to_python()
         x_range, y_range = self.get_plotrange(plotrange, start, stop)
         if not check_range(x_range) or not check_range(y_range):
             evaluation.message(self.get_name(), "prng", plotrange_option)
@@ -531,7 +531,7 @@ def check_exclusion(excl):
             return True
 
         exclusions_option = self.get_option(options, "Exclusions", evaluation)
-        exclusions = exclusions_option.to_python(n_evaluation=evaluation)
+        exclusions = eval_N(exclusions_option, evaluation).to_python()
         # TODO Turn expressions into points E.g. Sin[x] == 0 becomes 0, 2 Pi...
 
         if exclusions in ["System`None", ["System`None"]]:
@@ -1479,7 +1479,7 @@ def apply(self, points, evaluation, options):
         "%(name)s[points_, OptionsPattern[%(name)s]]"
 
         plot_name = self.get_name()
-        all_points = points.to_python(n_evaluation=evaluation)
+        all_points = eval_N(points, evaluation).to_python()
         # FIXME: arrange forself to have a .symbolname property or attribute
         expr = Expression(Symbol(self.get_name()), points, *options_to_rules(options))
 
@@ -1495,7 +1495,7 @@ def check_range(range):
             return False
 
         plotrange_option = self.get_option(options, "PlotRange", evaluation)
-        plotrange = plotrange_option.to_python(n_evaluation=evaluation)
+        plotrange = eval_N(plotrange_option, evaluation).to_python()
         if plotrange == "System`All":
             plotrange = ["System`All", "System`All"]
         elif plotrange == "System`Automatic":
@@ -1522,7 +1522,7 @@ def check_range(range):
         # Filling option
         # TODO: Fill between corresponding points in two datasets:
         filling_option = self.get_option(options, "Filling", evaluation)
-        filling = filling_option.to_python(n_evaluation=evaluation)
+        filling = eval_N(filling_option, evaluation).to_python()
         if filling in [
             "System`Top",
             "System`Bottom",
diff --git a/mathics/builtin/files_io/filesystem.py b/mathics/builtin/files_io/filesystem.py
index f24e5700b..6b13b7870 100644
--- a/mathics/builtin/files_io/filesystem.py
+++ b/mathics/builtin/files_io/filesystem.py
@@ -28,6 +28,7 @@
 )
 from mathics.core.convert.expression import to_expression, to_mathics_list
 from mathics.core.convert.python import from_python
+from mathics.core.evaluators import eval_N
 from mathics.core.expression import Expression
 from mathics.core.streams import (
     HOME_DIR,
@@ -830,9 +831,10 @@ def apply(self, path, timetype, evaluation):
             return
 
         # Offset for system epoch
-        epochtime = Expression(
+        epochtime_expr = Expression(
             SymbolAbsoluteTime, String(time.strftime("%Y-%m-%d %H:%M", time.gmtime(0)))
-        ).to_python(n_evaluation=evaluation)
+        )
+        epochtime = eval_N(epochtime_expr, evaluation).to_python()
         result += epochtime
 
         return to_expression("DateList", Real(result))
@@ -2149,7 +2151,7 @@ def apply(self, filename, datelist, attribute, evaluation):
         )
 
         stattime = to_expression("AbsoluteTime", from_python(py_datelist))
-        stattime = stattime.to_python(n_evaluation=evaluation)
+        stattime = eval_N(stattime, evaluation).to_python()
 
         stattime -= epochtime
 
diff --git a/mathics/builtin/numbers/numbertheory.py b/mathics/builtin/numbers/numbertheory.py
index 5ff142c83..ce0e5a027 100644
--- a/mathics/builtin/numbers/numbertheory.py
+++ b/mathics/builtin/numbers/numbertheory.py
@@ -494,8 +494,23 @@ class NextPrime(Builtin):
 
     def apply(self, n, k, evaluation):
         "NextPrime[n_?NumberQ, k_Integer]"
-        py_k = k.to_python(n_evaluation=evaluation)
-        py_n = n.to_python(n_evaluation=evaluation)
+
+        def to_int_value(x):
+            if isinstance(x, Integer):
+                return x.value
+            x = eval_N(x, evaluation)
+            if isinstance(x, Integer):
+                return x.value
+            elif isinstance(x, Real):
+                return round(x.value)
+            else:
+                return None
+
+        py_k = to_int_value(k)
+        if py_k is None:
+            return None
+
+        py_n = n.value
 
         if py_k >= 0:
             return Integer(sympy.ntheory.nextprime(py_n, py_k))
@@ -606,7 +621,7 @@ class PrimePi(SympyFunction):
 
     def apply(self, n, evaluation):
         "PrimePi[n_?NumericQ]"
-        result = sympy.ntheory.primepi(n.to_python(n_evaluation=evaluation))
+        result = sympy.ntheory.primepi(eval_N(n, evaluation).to_python())
         return Integer(result)
 
 
diff --git a/mathics/builtin/numbers/randomnumbers.py b/mathics/builtin/numbers/randomnumbers.py
index fd6ad81f1..aae92d365 100644
--- a/mathics/builtin/numbers/randomnumbers.py
+++ b/mathics/builtin/numbers/randomnumbers.py
@@ -17,10 +17,12 @@
 from mathics.builtin.base import Builtin
 from mathics.builtin.numpy_utils import instantiate_elements, stack
 from mathics.core.atoms import Integer, String, Real, Complex
+from mathics.core.evaluators import eval_N
 from mathics.core.expression import Expression
 from mathics.core.list import ListExpression
 from mathics.core.symbols import Symbol, SymbolDivide, SymbolNull
 
+
 SymbolRandomComplex = Symbol("RandomComplex")
 SymbolRandomReal = Symbol("RandomReal")
 SymbolTotal = Symbol("Total")
@@ -497,11 +499,21 @@ class RandomComplex(Builtin):
 
     @staticmethod
     def to_complex(value, evaluation):
-        result = value.to_python(n_evaluation=evaluation)
-        if isinstance(result, (float, int)):
-            result = complex(result)
-        if isinstance(result, complex):
-            return result
+        result = eval_N(value, evaluation)
+
+        if hasattr(result, "value"):
+            result_value = result.value
+        else:
+            # TODO: result.value does not work, because
+            # Complex does not have a ``value`` attribute.
+            # Otherwise, we could return here ``None``.
+            result_value = result.to_python()
+
+        if isinstance(result_value, (float, int)):
+            return complex(result_value)
+        if isinstance(result_value, complex):
+            return result_value
+
         return None
 
     def apply(self, zmin, zmax, evaluation):
@@ -644,9 +656,7 @@ def _weights_to_python(self, weights, evaluation):
                 return evaluation.message(self.get_name(), "wghtv", weights), None
             weights = norm_weights
 
-        py_weights = (
-            weights.to_python(n_evaluation=evaluation) if is_proper_spec else None
-        )
+        py_weights = eval_N(weights, evaluation).to_python() if is_proper_spec else None
         if (py_weights is None) or (
             not all(isinstance(w, (int, float)) and w >= 0 for w in py_weights)
         ):
diff --git a/mathics/core/convert/function.py b/mathics/core/convert/function.py
index b699137e4..551d3d21d 100644
--- a/mathics/core/convert/function.py
+++ b/mathics/core/convert/function.py
@@ -74,7 +74,7 @@ def _pythonized_mathics_expr(*x):
                 vars = dict(list(zip([a.name for a in args], x_mathics)))
                 pyexpr = expr.replace_vars(vars)
                 pyexpr = eval_N(pyexpr, inner_evaluation)
-                res = pyexpr.to_python(n_evaluation=inner_evaluation)
+                res = pyexpr.to_python()
                 return res
 
             # TODO: check if we can use numba to compile this...
diff --git a/mathics/core/element.py b/mathics/core/element.py
index 9a6e5cf41..4d0848dd6 100644
--- a/mathics/core/element.py
+++ b/mathics/core/element.py
@@ -422,12 +422,28 @@ def user_hash(self, update) -> None:
         # __hash__ might only hash a sample of the data available.
         raise NotImplementedError
 
-    def to_sympy(self, **kwargs):
+    def to_python(self, *args, python_form: bool = False, **kwargs):
+        # Returns a native builtin Python object
+        # something in (int, float, complex, str, tuple, list or dict.).
+        # (See discussions in
+        #  https://github.com/Mathics3/mathics-core/discussions/550
+        # and
+        # https://github.com/Mathics3/mathics-core/pull/551
+        #
+        #
+        # if n_evaluation is an Evaluation object, then the expression
+        # is passed by an eval_N().
+        # If python_form is True, the standard behaviour is changed,
+        # and it seems to behave like to_sympy....
+
         raise NotImplementedError
 
     def to_mpmath(self):
         raise NotImplementedError
 
+    def to_sympy(self, **kwargs):
+        raise NotImplementedError
+
 
 class EvalMixin:
     """
diff --git a/mathics/core/symbols.py b/mathics/core/symbols.py
index d023ec8ce..717617e5f 100644
--- a/mathics/core/symbols.py
+++ b/mathics/core/symbols.py
@@ -570,25 +570,47 @@ def sameQ(self, rhs: Any) -> bool:
         """Mathics SameQ"""
         return self is rhs
 
-    def to_python(self, *args, **kwargs):
-        # TODO: consider to return self.value if available.
-        # For general symbols, one possibility would be to
-        # return a sympy symbol, also stored in value.
-
+    def to_python(self, *args, python_form: bool = False, **kwargs):
         if self is SymbolTrue:
             return True
         if self is SymbolFalse:
             return False
         if self is SymbolNull:
             return None
+
+        # This was introduced before `mathics.core.evaluators.eval_N`
+        # provided a simple way to convert an expression into a number.
+        # Now it makes this routine harder to describe.
         n_evaluation = kwargs.get("n_evaluation")
         if n_evaluation is not None:
-            value = self.create_expression(SymbolN, self).evaluate(n_evaluation)
-            return value.to_python()
+            import warnings
+
+            warnings.warn(
+                "use instead ``eval_N(obj, evaluation).to_python()``",
+                DeprecationWarning,
+            )
+
+            from mathics.core.evaluators import eval_N
+
+            value = eval_N(self, n_evaluation)
+            if value is not self:
+                return value.to_python()
+
+        if python_form:
+            # TODO: consider to return self.value if available.
+            # For general symbols, one possibility would be to
+            # return a sympy symbol, also stored in value.
 
-        if kwargs.get("python_form", False):
+            # Also, why we need this parameter? If the idea is
+            # that to_python returns native (builtin) Python types,
+            # then to get a sympy object, we should use to_sympy.
             return self.to_sympy(**kwargs)
         else:
+            # For general symbols, the default behaviour is
+            # to return a 'str'. The reason seems to be
+            # that native (builtin) Python types
+            # are better for being used as keys in
+            # dictionaries.
             return self.name
 
     def to_sympy(self, **kwargs):

From 6a05acb72c62c4e33cf470e67e0c332f1ef4e7af Mon Sep 17 00:00:00 2001
From: mmatera <matera@fisica.unlp.edu.ar>
Date: Tue, 13 Sep 2022 16:38:18 -0300
Subject: [PATCH 2/4] removing python_form parameter from to_python

---
 mathics/builtin/inout.py | 7 ++++++-
 mathics/core/element.py  | 8 +-------
 mathics/core/symbols.py  | 1 +
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/mathics/builtin/inout.py b/mathics/builtin/inout.py
index f175ff397..4c614ef77 100644
--- a/mathics/builtin/inout.py
+++ b/mathics/builtin/inout.py
@@ -439,9 +439,14 @@ class PythonForm(Builtin):
     def apply_python(self, expr, evaluation) -> Expression:
         "MakeBoxes[expr_, PythonForm]"
 
+        def build_python_form(expr):
+            if isinstance(expr, Symbol):
+                return expr.to_sympy()
+            return expr.to_python()
+
         try:
             # from trepan.api import debug; debug()
-            python_equivalent = expr.to_python(python_form=True)
+            python_equivalent = build_python_form(expr)
         except Exception:
             return
         return StringFromPython(python_equivalent)
diff --git a/mathics/core/element.py b/mathics/core/element.py
index 4d0848dd6..56603c7f5 100644
--- a/mathics/core/element.py
+++ b/mathics/core/element.py
@@ -422,7 +422,7 @@ def user_hash(self, update) -> None:
         # __hash__ might only hash a sample of the data available.
         raise NotImplementedError
 
-    def to_python(self, *args, python_form: bool = False, **kwargs):
+    def to_python(self, *args, **kwargs):
         # Returns a native builtin Python object
         # something in (int, float, complex, str, tuple, list or dict.).
         # (See discussions in
@@ -430,12 +430,6 @@ def to_python(self, *args, python_form: bool = False, **kwargs):
         # and
         # https://github.com/Mathics3/mathics-core/pull/551
         #
-        #
-        # if n_evaluation is an Evaluation object, then the expression
-        # is passed by an eval_N().
-        # If python_form is True, the standard behaviour is changed,
-        # and it seems to behave like to_sympy....
-
         raise NotImplementedError
 
     def to_mpmath(self):
diff --git a/mathics/core/symbols.py b/mathics/core/symbols.py
index 717617e5f..115d25855 100644
--- a/mathics/core/symbols.py
+++ b/mathics/core/symbols.py
@@ -597,6 +597,7 @@ def to_python(self, *args, python_form: bool = False, **kwargs):
                 return value.to_python()
 
         if python_form:
+            1 / 0
             # TODO: consider to return self.value if available.
             # For general symbols, one possibility would be to
             # return a sympy symbol, also stored in value.

From c08f7e3e20f17858099e08c6b4e641d4d534e6c8 Mon Sep 17 00:00:00 2001
From: mmatera <matera@fisica.unlp.edu.ar>
Date: Tue, 13 Sep 2022 16:46:59 -0300
Subject: [PATCH 3/4] removing the trailing block of code

---
 mathics/core/symbols.py | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/mathics/core/symbols.py b/mathics/core/symbols.py
index 115d25855..aa17bb34c 100644
--- a/mathics/core/symbols.py
+++ b/mathics/core/symbols.py
@@ -596,23 +596,12 @@ def to_python(self, *args, python_form: bool = False, **kwargs):
             if value is not self:
                 return value.to_python()
 
-        if python_form:
-            1 / 0
-            # TODO: consider to return self.value if available.
-            # For general symbols, one possibility would be to
-            # return a sympy symbol, also stored in value.
-
-            # Also, why we need this parameter? If the idea is
-            # that to_python returns native (builtin) Python types,
-            # then to get a sympy object, we should use to_sympy.
-            return self.to_sympy(**kwargs)
-        else:
-            # For general symbols, the default behaviour is
-            # to return a 'str'. The reason seems to be
-            # that native (builtin) Python types
-            # are better for being used as keys in
-            # dictionaries.
-            return self.name
+        # For general symbols, the default behaviour is
+        # to return a 'str'. The reason seems to be
+        # that native (builtin) Python types
+        # are better for being used as keys in
+        # dictionaries.
+        return self.name
 
     def to_sympy(self, **kwargs):
         from mathics.builtin import mathics_to_sympy

From a9a09477a2de444da596004b7e7f6fa548f8209b Mon Sep 17 00:00:00 2001
From: mmatera <matera@fisica.unlp.edu.ar>
Date: Tue, 13 Sep 2022 17:11:17 -0300
Subject: [PATCH 4/4] another comment

---
 mathics/core/expression.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mathics/core/expression.py b/mathics/core/expression.py
index c3dae5233..df3f71fc6 100644
--- a/mathics/core/expression.py
+++ b/mathics/core/expression.py
@@ -1382,7 +1382,9 @@ def to_python(self, *args, **kwargs):
         """
         from mathics.builtin.base import mathics_to_python
 
-        n_evaluation = kwargs.get("n_evaluation")
+        n_evaluation = kwargs.get("n_evaluation", None)
+        assert n_evaluation is None
+
         head = self._head
         if head is SymbolFunction:
 
@@ -1416,6 +1418,9 @@ def to_python(self, *args, **kwargs):
             #         keywords=[],
             #         )
             return py_obj
+
+        # Notice that in this case, `to_python` returns a Mathics Expression object,
+        # instead of a builtin native object.
         return self
 
     def to_sympy(self, **kwargs):