-
-
Notifications
You must be signed in to change notification settings - Fork 65
Consolidate plot options processing into PlotOptions #1558
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
Conversation
commit 1819985 Merge: 4851b66 f80f79b Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Wed Dec 24 14:24:29 2025 -0500 Merge remote-tracking branch 'upstream/master' into dry-plot-options commit 4851b66 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Wed Dec 24 14:17:49 2025 -0500 Add comment commit 1fa1f19 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Wed Dec 24 13:59:33 2025 -0500 Formatting commit 47d919c Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Wed Dec 24 13:55:45 2025 -0500 Pass args to eval_Plot as a single PlotOptions commit d891f44 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Wed Dec 24 13:34:38 2025 -0500 Move Exclusions processing to PlotOptions commit 09542a0 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Wed Dec 24 12:47:46 2025 -0500 Add a test for Exclusions commit cc3eb1f Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Wed Dec 24 09:20:44 2025 -0500 Fix doctest commit d9b4b09 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Wed Dec 24 00:16:40 2025 -0500 Update doc-059-cls.txt commit 4f1c68e Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Dec 23 23:55:21 2025 -0500 Move PlotPoints option to PlotOptions commit 34aa1fc Merge: 9f6ba9b 9915441 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Dec 23 17:52:14 2025 -0500 Merge branch 'fix-spurious-log-axes' into dry-plot-options commit 9915441 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Dec 23 13:25:30 2025 -0500 Formattting commit d21ac5b Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Dec 23 13:24:48 2025 -0500 Distinguish between Graphics option LogPlot=False or True commit 9f6ba9b Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Dec 23 12:27:24 2025 -0500 Move Mesh option to PlotOptions, make it Symbol commit 39a6648 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Dec 23 11:50:52 2025 -0500 Use PlotOptions.plot_range in plot and plot3d commit c9306ed Merge: 9219999 0c46d96 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Dec 23 11:10:51 2025 -0500 Merge remote-tracking branch 'upstream/master' into dry-plot-options commit 9219999 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Dec 23 09:23:21 2025 -0500 Move plot_range processing to PlotOptions commit 2384e1e Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Dec 23 08:49:25 2025 -0500 Move PlotOptions out of plot_plot3d for general reuse commit 81146c8 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Dec 23 01:09:42 2025 -0500 WIP commit c734aa4 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Dec 23 01:07:22 2025 -0500 WIP commit da659bf Merge: 8d04f43 2ab9409 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Mon Dec 22 21:47:21 2025 -0500 Merge branch 'master' into dry-plot-options commit 2ab9409 Merge: 0df82b6 b5f7759 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Mon Dec 22 21:46:44 2025 -0500 Merge branch 'only-valid-graphics-options' commit 8d04f43 Merge: 284db15 68c24ee Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Mon Dec 22 21:41:36 2025 -0500 Merge remote-tracking branch 'upstream/master' into dry-plot-options commit 0df82b6 Merge: 5c2c3db 68c24ee Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Mon Dec 22 21:40:53 2025 -0500 Merge remote-tracking branch 'upstream/master' commit b5f7759 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Mon Dec 22 15:09:23 2025 -0500 Formatting commit 5281db5 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Mon Dec 22 14:59:55 2025 -0500 Make tests more robust * Only pass valid Graphics on from Plot to the resulting Graphics * Add (non-standard) LogPlot to list of valid Graphics options commit 284db15 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Mon Dec 22 14:20:17 2025 -0500 WIP commit 5c2c3db Merge: ba6332d 342f0ab Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Sun Dec 21 13:45:02 2025 -0500 Merge remote-tracking branch 'upstream/master' commit ba6332d Merge: 775c7b1 6e91c28 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Sun Dec 21 13:44:49 2025 -0500 Merge branch 'split-plots-builtins-by-base-class' commit 775c7b1 Merge: 48e0ad6 2208011 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Sun Dec 21 13:41:37 2025 -0500 Merge remote-tracking branch 'upstream/master' commit 6e91c28 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Sun Dec 21 11:22:29 2025 -0500 black commit ec4fc00 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Sun Dec 21 11:21:45 2025 -0500 black, isort, autoflake commit 068b673 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Sun Dec 21 11:15:39 2025 -0500 Split off plots that share _ base classes commit 48e0ad6 Merge: 0173596 7bb5b25 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Fri Dec 19 09:41:30 2025 -0500 Merge remote-tracking branch 'upstream/master' commit 0173596 Merge: ceb204a 68e1ae5 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Wed Dec 17 23:39:09 2025 -0500 Merge remote-tracking branch 'upstream/master' commit ceb204a Merge: b3f0277 73cbe0a Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Mon Dec 15 12:13:50 2025 -0500 Merge remote-tracking branch 'upstream/master' commit b3f0277 Merge: 4cf3048 a7acec0 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Fri Dec 12 13:49:38 2025 -0500 Merge remote-tracking branch 'upstream/master' commit 4cf3048 Merge: e57d5d9 d171d37 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Sun Dec 7 12:08:52 2025 -0500 Merge remote-tracking branch 'upstream/master' commit e57d5d9 Merge: e0d0ca0 1f5dbb2 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Sun Nov 30 22:11:46 2025 -0500 Merge remote-tracking branch 'upstream/master' commit e0d0ca0 Merge: 111a5ca 9481d26 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Thu Nov 27 09:55:07 2025 -0500 Merge remote-tracking branch 'upstream/master' commit 111a5ca Merge: 16be55c 6a09aab Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Nov 25 18:26:07 2025 -0500 Merge remote-tracking branch 'upstream/master' commit 16be55c Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Nov 25 14:55:03 2025 -0500 Update Combinatorica-repo commit 4f6d96f Merge: 16f5029 f8a4919 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Nov 25 14:54:28 2025 -0500 Merge remote-tracking branch 'upstream/master' commit 16f5029 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Mon Nov 24 12:01:39 2025 -0500 Revert "Update issue templates" This reverts commit 455e428. commit 81c8cb8 Merge: 7af7d4b 23a54d0 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Mon Nov 24 09:09:15 2025 -0500 Merge remote-tracking branch 'upstream/master' commit 7af7d4b Merge: e98c73d 6a3892f Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Sat Nov 22 23:39:50 2025 -0500 Merge remote-tracking branch 'upstream/master' commit e98c73d Merge: 4f17008 a1d40e1 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Nov 18 18:22:46 2025 -0500 Merge remote-tracking branch 'upstream/master' commit 4f17008 Merge: 3e897a1 c88706e Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Tue Nov 18 17:33:08 2025 -0500 Merge remote-tracking branch 'upstream/master' commit 3e897a1 Merge: 9db162f afed316 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Sun Nov 16 17:00:06 2025 -0500 Merge remote-tracking branch 'upstream/master' commit 9db162f Merge: bbaccf5 cc06072 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Sat Nov 15 09:15:35 2025 -0500 Merge remote-tracking branch 'upstream/master' commit bbaccf5 Merge: c8b277c 90d6767 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Wed Nov 12 08:25:31 2025 -0500 Merge remote-tracking branch 'upstream/master' commit c8b277c Merge: 455e428 c6e5d28 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Sun Nov 9 08:40:54 2025 -0500 Merge remote-tracking branch 'upstream/master' commit 455e428 Author: Bruce Lucas <bdlucas1@users.noreply.github.com> Date: Fri Nov 7 11:21:49 2025 -0500 Update issue templates
This test has given us problems before because on some platforms the 3 shows up as int and on others as float, presumably due to low-level library differences. This difference is inconsequential to the function. This change will cause all platforms to use float, allowing us to keep the test.
|
Please let's add docstrings and sort orders, where needed, to the top of |
|
Added the docstrings, and also un-DRYed the sort_order thing (although unless the doc system is examining the text of the modules or parsing it to an AST I don't think it would see any difference). |
|
|
||
| # This tells documentation how to sort this module | ||
| from .plot import sort_order # noqa | ||
| sort_order = "mathics.builtin.plotting-data" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you just remove this line and the other lines like this altogether, the right thing will happen since mathics.drawing.plot_listplot comes lexically before mathics.drawing.plot_plot which comes lexically before mathics.drawing.plot_plot3d.
I don't think there is particularly any user-centric meaning to those modules - for example, ContourPlot and ComplexPlot3D are both in plot_plot3d.py because they share implementation (_Plot3D base class), but that has no significance to an end-user. If module structure dictates document structure then probably some rearrangement of module structure will be needed here. This PR is already large - can we keep it focused on the code structure and solve the #1557 documentation issue in a separate future PR? |
Well, they are both 3-parameter-valued plots shown on a 2D screen, and that's why they share the same code. I'm okay with saying something along those lines and leaving the rearrangement of modules or finer points for later. In the case of charts and list plots, the user-centric description though, is clear.
Personally, I do use the documentation, such as it is, and when it is broken like this, it makes it harder for me to figure out what's up. When things get inadvertently broken as a result of an inattention to detail, I would hope there's a priority for addressing this over going further down the rewrite structure, and possibly as an afterthought we fix up the stuff that got broken along the way. (Unless, of course, we know that this kind of thing is going to happen and we agree to let things stay broken until the end.) It is not that hard to write something reasonable here. If you don't want to do it, let me know, and I'll suggest some text to add. I don't think it's all that difficult a thing to do. Please let me know your thoughts. |
|
Feel free update the PR with whatever text will help you. |
Oh, that reminds me of one other thing... when you put in PRs, it helps if you do it in a branch in this repository (as opposed to bdlucas1). It makes this kind of suggesting changes to the PR easier. Thanks. |
OK, will do. At one point I think I didn't have the ability to do that so just did it as a branch in my fork. Will look into how to do it as a branch in this repo. |
| doc-059: | ||
| # The original doctest had 3 instead of 3.1, but that would give int on some platforms | ||
| # and float on others. So changed here to 3.1 to force float on all platforms. | ||
| # TODO: consider making test insensitive to float vs int provided the value is the same. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@rocky when you've had a chance to come up with the text you want let me know and I'll update the PR. |
I'll add the text as a PR to your PR after you've rebased this from this repository. Thanks. |
Sorry, I don't know what that means exactly or how to do it - I'm not very facile with github branches. I guess you're referring to your previous comment about submitting PRs using branches from this repo, but I thought we were talking about future PRs. I have figured out how to do that and will do so in the future. Can we continue with this PR in its current form? |
I take that part |
Thanks @mmatera ! |
Just rebase #1558 --------- Co-authored-by: Bruce Lucas <bdlucas1@users.noreply.github.com> Co-authored-by: R. Bernstein <rocky@users.noreply.github.com>

Sorry about the size of this PR - I tried to find a way to do it more incrementally, but things were too tangled for me to figure out how to do that.
The main thrust of this PR is to consolidate options processing for plots in a common class, PlotOptions, eliminating as much duplicate code as possible. PlotOptions was created initially for and shared by the 3d plots that inherit from _Plot3D in plot_plots3d.py. With this PR it has been generalized and is also used by the plots in plot_plots.py that inherit from _Plot.
(With one exception: DiscretePlot is in plot_plots.py and inherits from _Plot, but it calls eval_ListPlot. TBD whether or how that can be untangled in a future PR).
I also tried to move the Python data structures in PlotOptions to use Symbols instead of strings. To facilitate this I added a keyword argument, preserve_symbols, to to_python. The effect is Symbols are not converted to strings if this arg is True.
All tests pass and I've also verified locally through automated testing that images generated from the Plots for the doctest tests (and more) both via Plotly (m3d) and via SVG are unchanged.