Refactor plot3d (#1520)

This PR substantially refactors Plot3D and DensityPlot (which share a
good bit of code), in preparation for adding a new vectorized
implementation of those two functions.  No function is changed or
added.

* The the flow from builtin to eval is cleaner - no more callbacks
  from eval to builtin. Options processing is done prior to calling
  eval_*. Plot3D and DensityPlot now have their own eval functions
  which share implementation code in eval rather than in builtin.

* A PlotOptions class has been introduced. This simplifies passing
  plotting options from builtin to eval. It may also be reusable for
  other plotting builtins.

* A new GraphicsGenerator class has been added. This consolidates and
  regularizes some scattered code for generating Graphics and
  Graphics3D expressions, which are rather complex in structure. This
  will be further enhanced to support GraphicsComplex in the PR that
introduces vectorized plotting functions. Possibly could be retrofitted
  to other plotting functions.

* A test for DensityPlot was added, which was missed in the previous
  PR because I didn't realize how much code it shared with Plot3D.

This is a fairly big change, but I worked carefully, frequently
testing using the detailed tests for Plot3D and DensityPlot.  The
tests caught numerous errors as I worked, increasing confidence that
the warning light was hooked up and no functional changes have been
introduced.

Some quick timing tests show no change in performance as far as I can
see.

More can be done, particularly in the areas of error checking and
typing, but that shouldn't change the PR in any major way so hoping
to get some eyes on it in parallel.

Author: bdlucas1

mathics/builtin/drawing/plot.py

@@ -27,13 +27,14 @@
 from mathics.core.list import ListExpression
 from mathics.core.symbols import Symbol, SymbolList
 from mathics.core.systemsymbols import (
+    SymbolAll,
     SymbolBlack,
     SymbolEdgeForm,
+    SymbolFull,
     SymbolGraphics,
-    SymbolGraphics3D,
     SymbolLine,
     SymbolLog10,
-    SymbolPolygon,
+    SymbolNone,
     SymbolRGBColor,
     SymbolStyle,
 )
@@ -50,7 +51,7 @@
     get_plot_range,
     get_plot_range_option,
 )
-from mathics.eval.drawing.plot3d import construct_density_plot, eval_plot3d
+from mathics.eval.drawing.plot3d import eval_DensityPlot, eval_Plot3D
 from mathics.eval.nevaluator import eval_N
 
 # This tells documentation how to sort this module
@@ -407,7 +408,117 @@ def process_function_and_options(
         return functions, x_name, py_start, py_stop, x_range, y_range, expr_limits, expr
 
 
+# TODO: add more options
+# TODO: generalize, use for other plots
+class PlotOptions:
+    """
+    Extract Options common to many types of plotting.
+    This aims to reduce duplication of code,
+    and to make it easier to pass options to eval_* routines.
+    """
+
+    # TODO: more precise types
+    ranges: list
+    mesh: str
+    plotpoints: list
+    maxdepth: int
+
+    def error(self, what, *args, **kwargs):
+        if not isinstance(what, str):
+            what = what.get_name()
+        self.evaluation.message(what, *args, **kwargs)
+        raise ValueError()
+
+    def __init__(self, expr, range_exprs, options, evaluation):
+        self.evaluation = evaluation
+
+        # plot ranges
+        self.ranges = []
+        for range_expr in range_exprs:
+            if not range_expr.has_form("List", 3):
+                self.error(expr, "invrange", range_expr)
+            if not isinstance(range_expr.elements[0], Symbol):
+                self.error(expr, "invrange", range_expr)
+            range = [range_expr.elements[0]]
+            for limit_expr in range_expr.elements[1:3]:
+                limit = limit_expr.round_to_float(evaluation)
+                if limit is None:
+                    self.error(expr, "plln", limit_expr, range_expr)
+                range.append(limit)
+            if range[2] <= range[1]:
+                self.error(expr, "invrange", range_expr)
+            self.ranges.append(range)
+
+        # Mesh option
+        mesh = expr.get_option(options, "Mesh", evaluation)
+        if mesh not in (SymbolNone, SymbolFull, SymbolAll):
+            evaluation.message("Mesh", "ilevels", mesh)
+            mesh = SymbolFull
+        self.mesh = mesh
+
+        # PlotPoints option
+        plotpoints_option = expr.get_option(options, "PlotPoints", evaluation)
+        plotpoints = plotpoints_option.to_python()
+
+        def check_plotpoints(steps):
+            if isinstance(steps, int) and steps > 0:
+                return True
+            return False
+
+        if plotpoints == "System`None":
+            plotpoints = (7, 7)
+        elif check_plotpoints(plotpoints):
+            plotpoints = (plotpoints, plotpoints)
+        if not (
+            isinstance(plotpoints, (list, tuple))
+            and len(plotpoints) == 2
+            and check_plotpoints(plotpoints[0])
+            and check_plotpoints(plotpoints[1])
+        ):
+            evaluation.message(expr.get_name(), "invpltpts", plotpoints)
+            plotpoints = (7, 7)
+        self.plotpoints = plotpoints
+
+        # MaxRecursion Option
+        maxrec_option = expr.get_option(options, "MaxRecursion", evaluation)
+        max_depth = maxrec_option.to_python()
+        if isinstance(max_depth, int):
+            if max_depth < 0:
+                max_depth = 0
+                evaluation.message(expr.get_name(), "invmaxrec", max_depth, 15)
+            elif max_depth > 15:
+                max_depth = 15
+                evaluation.message(expr.get_name(), "invmaxrec", max_depth, 15)
+            else:
+                pass  # valid
+        elif max_depth == float("inf"):
+            max_depth = 15
+            evaluation.message(expr.get_name(), "invmaxrec", max_depth, 15)
+        else:
+            max_depth = 0
+            evaluation.message(expr.get_name(), "invmaxrec", max_depth, 15)
+        self.max_depth = max_depth
+
+        # ColorFunction and ColorFunctionScaling options
+        # This was pulled from construct_density_plot (now eval_DensityPlot).
+        # TODO: What does pop=True do? is it right?
+        # TODO: can we move some of the subsequent processing in eval_DensityPlot to here?
+        # TODO: what is the type of these? that may change if we do the above...
+        self.color_function = expr.get_option(
+            options, "ColorFunction", evaluation, pop=True
+        )
+        self.color_function_scaling = expr.get_option(
+            options, "ColorFunctionScaling", evaluation, pop=True
+        )
+
+
 class _Plot3D(Builtin):
+    """Common base class for Plot3D and DensityPlot"""
+
+    # Check for correct number of args
+    eval_error = Builtin.generic_argument_error
+    expected_args = 3
+
     messages = {
         "invmaxrec": (
             "MaxRecursion must be a non-negative integer; the recursion value "
@@ -422,25 +533,35 @@ class _Plot3D(Builtin):
             "Value of PlotPoints -> `1` is not a positive integer "
             "or appropriate list of positive integers."
         ),
+        "invrange": (
+            "Plot range `1` must be of the form {variable, min, max}, "
+            "where max > min."
+        ),
     }
 
     def eval(
         self,
         functions,
-        x,
-        xstart,
-        xstop,
-        y,
-        ystart,
-        ystop,
+        xrange,
+        yrange,
         evaluation: Evaluation,
         options: dict,
     ):
-        """%(name)s[functions_, {x_Symbol, xstart_, xstop_},
-        {y_Symbol, ystart_, ystop_}, OptionsPattern[%(name)s]]"""
-        return eval_plot3d(
-            self, functions, x, xstart, xstop, y, ystart, ystop, evaluation, options
-        )
+        """%(name)s[functions_, xrange_, yrange_, OptionsPattern[%(name)s]]"""
+
+        # TODO: test error for too many, too few, no args
+
+        # parse options, bailing out if anything is wrong
+        try:
+            plot_options = PlotOptions(self, [xrange, yrange], options, evaluation)
+        except ValueError:
+            return None
+
+        # ask the subclass to get one or more functions as appropriate
+        plot_options.functions = self.get_functions_param(functions)
+
+        # delegate to subclass, which will call the appropriate eval_* function
+        return self.do_eval(plot_options, evaluation, options)
 
 
 class BarChart(_Chart):
@@ -563,7 +684,6 @@ class ColorDataFunction(Builtin):
     """
 
     summary_text = "color scheme object"
-    pass
 
 
 class DensityPlot(_Plot3D):
@@ -608,22 +728,17 @@ class DensityPlot(_Plot3D):
     )
     summary_text = "density plot for a function"
 
+    # TODO: error if more than one function here
     def get_functions_param(self, functions):
+        """can only have one function"""
         return [functions]
 
-    def construct_graphics(
-        self, triangles, mesh_points, v_min, v_max, options, evaluation
-    ):
-        return construct_density_plot(
-            self, triangles, mesh_points, v_min, v_max, options, evaluation
-        )
-
-    def final_graphics(self, graphics, options):
-        return Expression(
-            SymbolGraphics,
-            ListExpression(*graphics),
-            *options_to_rules(options, Graphics.options),
-        )
+    # called by superclass
+    def do_eval(self, plot_options, evaluation, options):
+        """called by superclass to call appropriate eval_* function"""
+        graphics = eval_DensityPlot(plot_options, evaluation)
+        graphics_expr = graphics.generate(options_to_rules(options, Graphics.options))
+        return graphics_expr
 
 
 class DiscretePlot(_Plot):
@@ -1753,43 +1868,14 @@ class Plot3D(_Plot3D):
     summary_text = "plots 3D surfaces of one or more functions"
 
     def get_functions_param(self, functions):
+        """May have a function or a list of functions"""
         if functions.has_form("List", None):
             return functions.elements
         else:
             return [functions]
 
-    def construct_graphics(
-        self, triangles, mesh_points, v_min, v_max, options, evaluation: Evaluation
-    ):
-        graphics = []
-        for p1, p2, p3 in triangles:
-            graphics.append(
-                Expression(
-                    SymbolPolygon,
-                    ListExpression(
-                        to_mathics_list(*p1),
-                        to_mathics_list(*p2),
-                        to_mathics_list(*p3),
-                    ),
-                )
-            )
-        # Add the Grid
-        for xi in range(len(mesh_points)):
-            line = []
-            for yi in range(len(mesh_points[xi])):
-                line.append(
-                    to_mathics_list(
-                        mesh_points[xi][yi][0],
-                        mesh_points[xi][yi][1],
-                        mesh_points[xi][yi][2],
-                    )
-                )
-            graphics.append(Expression(SymbolLine, ListExpression(*line)))
-        return graphics
-
-    def final_graphics(self, graphics, options: dict):
-        return Expression(
-            SymbolGraphics3D,
-            ListExpression(*graphics),
-            *options_to_rules(options, Graphics3D.options),
-        )
+    def do_eval(self, plot_options, evaluation, options):
+        """called by superclass to call appropriate eval_* function"""
+        graphics = eval_Plot3D(plot_options, evaluation)
+        graphics_expr = graphics.generate(options_to_rules(options, Graphics3D.options))
+        return graphics_expr

mathics/core/systemsymbols.py

@@ -104,6 +104,7 @@
 SymbolFloor = Symbol("System`Floor")
 SymbolFormat = Symbol("System`Format")
 SymbolFractionBox = Symbol("System`FractionBox")
+SymbolFull = Symbol("System`Full")
 SymbolFullForm = Symbol("System`FullForm")
 SymbolFunction = Symbol("System`Function")
 SymbolGamma = Symbol("System`Gamma")
@@ -291,6 +292,7 @@
 SymbolUpValues = Symbol("System`UpValues")
 SymbolVariance = Symbol("System`Variance")
 SymbolVerbatim = Symbol("Verbatim")
+SymbolVertexColors = Symbol("VertexColors")
 SymbolWhitespace = Symbol("System`Whitespace")
 SymbolWhitespaceCharacter = Symbol("System`WhitespaceCharacter")
 SymbolWord = Symbol("System`Word")