Refactor plot3d #1520

bdlucas1 · 2025-11-13T19:52:06Z

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.

Also goes through Plot3D code

This test was in doctest. Copied to test_plot so test failure could be caught earlier.

Also copied test to test_plot for earlier detection.

rocky · 2025-11-13T23:35:57Z

mathics/builtin/drawing/plot.py

-        )
+        """%(name)s[args___, OptionsPattern[%(name)s]]"""
+
+        # TODO: test error for too many, too few, no args


FYI setting a class variable expected_args is the simple way we add limit checking to Builtin functions, that give the messages with argx, argr, argrx, argt, or argtu tags. git grep -n expected_args to see examples of this use.

OK, thanks. Did that, and also added some other error checking for the range args.

rocky · 2025-11-13T23:39:55Z

mathics/builtin/drawing/plot.py

+        # Mesh option
+        mesh_option = expr.get_option(options, "Mesh", evaluation)
+        mesh = mesh_option.to_python()
+        if mesh not in ["System`None", "System`Full", "System`All"]:


What is typically done here is not to use to_python(), but instead do checks against the symbols [SymbolNone, SymbolAll, Symbol("System``Full]. The first two Symbols are already defined in mathics.atoms.systemsymbols; SymbolFull should probably be added.

Just to clarify - do you favor passing it to eval as a Symbol instead of a string?

Yes, Symbols are better to describe intent as well as being slightly faster.

It is also possible one day that might be able to pickle everything in mathics.core.systemsymbols to make loading fast. But that and autoload like GNU Emacs has is a different topic.

Agreed. Also, it's safer because typos are caught.

I think it would be good to encourage use of Symbols instead of strings. I don't know why the original coder might have used strings, but if I were writing the code I might have done the same thing because just typing a string is a lot easier than using a symbol, figuring out where it's defined (symbols or systemsymbols), and adjusting the import.

Here are some things I would find easier, in order of decreasing ease:

# no import to adjust, no redundant Symbol, all symbols are in the same place import system.symbols as sym ... sym.Foo ... # no imports to adjust, all symbols are in the same place, redundant Symbol isn't that bad import system.symbols as sym ... sym.SymbolFoo ... # all symbols are in the same import, not split between symbols and systemsymbols from system.symbols import SymbolFoo ... SymbolFoo ...

In my view any of those would be an improvement. I understand though it might be a substantial change, and may not be judged worth it.

Forgot to mention, I made the change.

Agreed. Also, it's safer because typos are caught.

I think it would be good to encourage use of Symbols instead of strings. I don't know why the original coder might have used strings, but if I were writing the code I might have done the same thing because just typing a string is a lot easier than using a symbol, figuring out where it's defined (symbols or systemsymbols), and adjusting the import.

Here are some things I would find easier, in order of decreasing ease:

# no import to adjust, no redundant Symbol, all symbols are in the same place import system.symbols as sym ... sym.Foo ... # no imports to adjust, all symbols are in the same place, redundant Symbol isn't that bad import system.symbols as sym ... sym.SymbolFoo ... # all symbols are in the same import, not split between symbols and systemsymbols from system.symbols import SymbolFoo ... SymbolFoo ...

In my view any of those would be an improvement. I understand though it might be a substantial change, and may not be judged worth it.

I am okay with the first or any of these.

@mmatera - your opinion?

I find it amusing to see "didn't think of that" roles reversed here.

An old Systems-Administration saying:

Never ascribe to maliciousness that which can easily be explained by ignorance or thoughtlessness.

rocky · 2025-11-14T00:06:53Z

test/builtin/drawing/test_plot.py

+            PlotRangePadding -> {1,2},
+            TicksStyle -> {RGBColor[0.5,0,0.5],GrayLevel[1]}
+        ]
+        """,


For what it's worth, when I run this in either the Django or mathicsscript interfaces, I am getting a BoxExpressionError

... File "/src/external-vcs/github/bdlucas1/mathics-core/mathics/builtin/box/graphics.py", line 612, in _prepare_elements xmin, xmax, ymin, ymax, w, h, width, height = calc_dimensions(final_pass=False) ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^ File "/src/external-vcs/github/bdlucas1/mathics-core/mathics/builtin/box/graphics.py", line 566, in calc_dimensions raise BoxExpressionError ```

Was that in master or this PR or both?

Was that in master or this PR or both?

I just tried on master. I get the same BoxExpressionError. The code we currently have is very fragile.

OK. Fragility aside, I don't know that those are valid plots - the intent was just to check whether options are passed through correctly.

I have been thinking about how to do some end-to-end testing where valid plots are compared visually. In some ways those tests will be easier to do, though maybe harder to debug. I think that's outside the scope of core though.

rocky · 2025-11-14T00:08:22Z

Much needed work. Thanks for undertaking. I will look at and see if I can answer the questions in the code in another pass, probably tomorrow.

rocky · 2025-11-14T13:04:08Z

mathics/builtin/drawing/plot.py

+
+        # ColorFunction and ColorFunctionScaling options
+        # This was pulled from construct_density_plot (now eval_DensityPlot).
+        # TODO: What does pop=True do? is it right?


Here's a guess. A dictionary method "pop" is used to remove an element from a dictionary. Since that's some sort of option, I think the idea is about whether this opiton is fully handled at this point and therefore remove it from the list of options, or keep it in the options dictionary for other routines to adjust behavior based on looking in the dictionary (as opposed to consulting a variable or some other state).

Just a guess though.

Makes sense as to what it does, but not sure why it's done only in this case. Anyway at some point I want to take a pass over the complete list of plotting options because currently only a subset are handled, and maybe as part of that I'll try to make handling more uniform. Getting options processing pulled out into its own thing was a first step.

rocky · 2025-11-14T13:11:29Z

mathics/builtin/drawing/plot.py

+        # 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?


I realize this may be a rhetorical question, but I sometimes answer those. Yes, I hope so. The code organization here is currently a mess.

Appreciate your response. I may look into that refactoring as part of the pass over all plotting options that I mentioned.

rocky · 2025-11-14T13:34:39Z

test/builtin/drawing/test_plot.py

+                    }
+                ]
+            },
+            AspectRatio -> 1, (* TODO: is not passed through apparently - or is my misunderstanding? *)


Here is what I am seeing through experimentation. AspectRatio is handled properly for the simpler example:

Graphics[ Polygon[ {{{0.0,0.0},{0.0,0.5},{0.5,0.0}}} ], AspectRatio->0.5 ]

The VertexColors option has not been added to Mathics3.

Also, when a list of polygons is given, I am not seeing anything, which could be a boxing roblem — in neither instance am no I getting an error reported.

Enhanced PlotOptions to validate plot range format and ensure max > min, raising descriptive errors for invalid input. Updated _Plot3D to use explicit arguments and improved error messaging. Added new test cases to verify error handling for invalid Plot3D arguments and ranges.

rocky · 2025-11-14T14:41:00Z

test/builtin/drawing/test_plot.py

            None,
        ),
+        ("Plot3D[]", ("Plot3D called with 0 arguments; 3 arguments are expected.",), "Plot3D[]", None),
+        ("Plot3D[1, 2, 3]", ("Plot range 2 must be of the form {variable, min, max}, where max > min.",), "Plot3D[1, 2, 3]", None),


Kind of a minor thing, but the error I see in WL 13.2.0 for this expression and the next one has a "pllim" tag instead of an "invrange" tag that says "Range specification 2 is not of the form {x, xmin, xmax}.

It might be of interest in that it may give some insight as to how Mathematica is working.

I took a quick look at it's error handling in for Plot3D ranges, and got some funky results for some of the mis-specified ranges. My conclusion is that their error handling is just as much an inconsistent mess as ours (and I think you said much the same in your email), so I think it's not worth putting excessive effort into emulating it in detail. For that matter, is there any reason to think they consider error messages a stable interface from version to version?

In this case I thought a simple generic error message would be low-effort and would suffice.

I took a quick look at it's error handling in for Plot3D ranges, and got some funky results for some of the mis-specified ranges.

Great - thanks for doing this.

My conclusion is that their error handling is just as much an inconsistent mess as ours (and I think you said much the same in your email), so I think it's not worth putting excessive effort into emulating it in detail. For that matter, is there any reason to think they consider error messages a stable interface from version to version?

Dunno. @mmatera you're more of an expert here.

In this case I thought a simple generic error message would be low-effort and would suffice.

I'm okay with this. As might be obvious, when one kind of at the bottom, one is not looking for perfection, just improvement.

If it is ok for you, I will going over this during the weekend
M

Great, thanks!

rocky

LGTM. If you want to address the message tags difference, okay. This is a marked improvement over what we have right now, even if functionality is about the same (but with better error checking).

@mmatera Any thoughts?

rocky · 2025-11-14T14:54:47Z

mathics/eval/drawing/plot3d.py

        # add the mesh
        mesh_points = []
-        if mesh == "System`Full":
+        if mesh == SymbolFull:


Actually, now that Symbols are used, all of the == tests can become is tests. (Symbols are unique). Some linters will warn of this, I guess because "is" is faster.

thanks, done

bdlucas1 added 21 commits November 12, 2025 11:02

Add test for DensityPlot

d99778b

Also goes through Plot3D code

Formatting

3e2c935

move option processing from eval back to builtin

d201f0a

Add test to ensure mesh lines are generated

bd6e586

plot_options as structure

1a1398a

First cut at PlotOptions

299066e

WIP (less DensityPlot)

b0ccce1

WIP add DensityPlot

0105427

Cleanup

02a4d57

Cleanup

e6e36e1

SymbolVertexColors

5a65b4a

Cleanup

e4e41f0

WIP checkpoint

36b153d

WIP

04c621f

WIP

1e51796

Move GraphicsGenerator to eval

39c3c90

autoflake

5c5e089

Formatting

0f6b036

Cleanup

fdf3aca

Pull color options out into PlotOptions

f715099

Formatting

41e9992

bdlucas1 requested a review from rocky November 13, 2025 19:52

bdlucas1 added 3 commits November 13, 2025 17:53

Add test

9afaa9d

This test was in doctest. Copied to test_plot so test failure could be caught earlier.

Fix failing doctest

36cabbf

Also copied test to test_plot for earlier detection.

Formatting

9a9a85a

rocky reviewed Nov 13, 2025

View reviewed changes

rocky reviewed Nov 14, 2025

View reviewed changes

bdlucas1 added 3 commits November 14, 2025 09:15

Formatting

fa42e58

Cleanup

a1e3c59

rocky reviewed Nov 14, 2025

View reviewed changes

rocky approved these changes Nov 14, 2025

View reviewed changes

bdlucas1 added 2 commits November 14, 2025 09:47

Use Symbol instead of string for mesh option

68e0cd6

Formatting

1c078ff

rocky reviewed Nov 14, 2025

View reviewed changes

Use is instead of ==

10d4f33

Uh oh!

Refactor plot3d #1520

Are you sure you want to change the base?

Refactor plot3d #1520

Uh oh!

Conversation

bdlucas1 commented Nov 13, 2025

Uh oh!

rocky Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocky Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocky commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rocky Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocky Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocky Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocky Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

rocky Nov 13, 2025 •

edited

Loading

rocky Nov 13, 2025 •

edited

Loading

rocky commented Nov 14, 2025 •

edited

Loading

rocky Nov 14, 2025 •

edited

Loading

rocky Nov 14, 2025 •

edited

Loading

rocky Nov 14, 2025 •

edited

Loading

rocky Nov 14, 2025 •

edited

Loading